Bug 248955 - [PATCH] net80211: fix ieee80211_media_change() return value
Summary: [PATCH] net80211: fix ieee80211_media_change() return value
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Bjoern A. Zeeb
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-28 03:25 UTC by Tong Zhang
Modified: 2020-09-11 14:19 UTC (History)
2 users (show)

See Also:
bz: mfc-stable12?


Attachments
patch (843 bytes, patch)
2020-08-28 03:25 UTC, Tong Zhang
no flags Details | Diff
v2 patch (3.67 KB, patch)
2020-09-01 22:13 UTC, Tong Zhang
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tong Zhang 2020-08-28 03:25:17 UTC
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
Comment 1 Bjoern A. Zeeb freebsd_committer 2020-09-01 21:08:58 UTC
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.
Comment 2 Tong Zhang 2020-09-01 21:34:28 UTC
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
Comment 3 Tong Zhang 2020-09-01 22:13:38 UTC
Created attachment 217697 [details]
v2 patch

V2: change the predicate in callers instead of changing return code in callee
Comment 4 commit-hook freebsd_committer 2020-09-07 15:36:38 UTC
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
Comment 5 Bjoern A. Zeeb freebsd_committer 2020-09-07 15:37:48 UTC
I further simplified some code and committed the changes and will merged them before 12.2.
Comment 6 commit-hook freebsd_committer 2020-09-11 14:19:27 UTC
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