Created attachment 217578 [details] patch it seems that there are multiple caller checking the successful execution of ieee80211_media_change() using ENETRESET, but ieee80211_media_change() can only return 0 and EINVAL
I am not sure this is the correct fix. A lot of drivers also pass it to ieee80211_vap_attach() which then tickles down to ifmedia_init() where it is set to ifm->ifm_change = change_callback; ifmedia_ioctl() then checks for a != 0 error. Seems to be a good copy&paste error either way as one or the other case will break. It seems in r178354 the first factoring out of things happened, when it could still return ENETRESET which was then after successive changes removed in https://svnweb.freebsd.org/base/head/sys/net80211/ieee80211.c?r1=193339&r2=193340& which is the code as it is today. As a conclusion I'd say the drivers have not been updated to reflect these changes after r178354; see https://svnweb.freebsd.org/base/head/sys/dev/ath/if_ath.c?r1=178353&r2=178354& for why this still was. Would you follow the conclusion that these days the drivers could savely check for != 0 and we should rather fix (the ones I found): ./dev/ath/if_ath.c ./dev/bwi/if_bwi.c ./dev/iwm/if_iwm.c ./dev/iwn/if_iwn.c ./dev/malo/if_malo.c ./dev/mwl/if_mwl.c ./dev/otus/if_otus.c ./dev/usb/wlan/if_run.c ./dev/wtap/if_wtap.c Cc:ing re-assigning back to the list so that other driver maintainers will also see.
Thanks Bjoern! I just found out the code looks strange -- and yes I also noticed those places you pointed out. I think checking !=0 should work. The original patch I proposed makes minimal changes instead of touching multiple files. I will send out another revision. Thank you! - Tong
Created attachment 217697 [details] v2 patch V2: change the predicate in callers instead of changing return code in callee
A commit references this bug: Author: bz Date: Mon Sep 7 15:35:42 UTC 2020 New revision: 365419 URL: https://svnweb.freebsd.org/changeset/base/365419 Log: WiFi: fix ieee80211_media_change() callers In r178354 with the introduction of multi-bss ("vap") support factoring out started and with r193340 ieee80211_media_change() no longer returned ENETRESET but only 0 or error. As ieee80211(9) tells the ieee80211_media_change() function should not be called directly but is registered with ieee80211_vap_attach() instead. Some drivers have not been fully converted. After fixing the return checking some of these functions were simply wrappers between ieee80211_vap_attach() and ieee80211_media_change(), so remove the extra function, where possible as well. PR: 248955 Submitted by: Tong Zhang (ztong0001 gmail.com) (original) MFC after: 3 days Sponsored by: The FreeBSD Foundation Changes: head/sys/dev/ath/if_ath.c head/sys/dev/bwi/if_bwi.c head/sys/dev/iwm/if_iwm.c head/sys/dev/iwn/if_iwn.c head/sys/dev/mwl/if_mwl.c head/sys/dev/otus/if_otus.c head/sys/dev/usb/wlan/if_run.c head/sys/dev/wtap/if_wtap.c
I further simplified some code and committed the changes and will merged them before 12.2.
A commit references this bug: Author: bz Date: Fri Sep 11 14:18:47 UTC 2020 New revision: 365633 URL: https://svnweb.freebsd.org/changeset/base/365633 Log: iwm: fix regression from r365419 (ieee80211_media_change()) In r365419 ieee80211_media_change() callers were updated to not longer act on the obselete ENETRESET return code. While in the old days iwm has done a stop/init cycle in these cases, this was not executed since r193340. As a consequence simplify iwm code as well by passing ieee80211_media_change() right to ieee80211_vap_attach() as there is no more need for a local implementation. Reported by: Tomoaki AOKI (junchoon dec.sakura.ne.jp) Tested by: Tomoaki AOKI (junchoon dec.sakura.ne.jp) MFC after: 3 days X-MFC: fix is already in stable/12 PR: 248955 Changes: head/sys/dev/iwm/if_iwm.c
A commit references this bug: Author: bz Date: Thu Nov 5 11:56:49 UTC 2020 New revision: 367371 URL: https://svnweb.freebsd.org/changeset/base/367371 Log: MFC r365633: iwm: fix regression from r365419 (ieee80211_media_change()) In r365419 ieee80211_media_change() callers were updated to not longer act on the obselete ENETRESET return code. While in the old days iwm has done a stop/init cycle in these cases, this was not executed since r193340. As a consequence simplify iwm code as well by passing ieee80211_media_change() right to ieee80211_vap_attach() as there is no more need for a local implementation. The actual problem was fixed in stable/12 since r365608; this just reduces changes to HEAD. PR: 248955 Changes: _U stable/12/ stable/12/sys/dev/iwm/if_iwm.c