| Message ID | 1398428634-13394-1-git-send-email-tomasz.bursztyka@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
> -----Original Message----- > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > bounces@lists.shmoo.com] On Behalf Of Tomasz Bursztyka > Sent: Friday, April 25, 2014 15:24 > To: hostap@lists.shmoo.com > Subject: [PATCH] nl80211: Do not assume the device is rf-killed before trying > to set it up > > This fixes a regression which is assuming that since the general rf-kill status is > on, all devices are rf-killed. This rule is actually not true since this rf-kill status > is not per-device specific and the actual device might not be rf-killed though > users are prevented to use it due to that logic. > > The kernel knows better which device is rf-killed or not, and will check its > status properly when it tries to set it up (or "start" it for P2P). > Thus it's better to rely on the kernel's returned code as it used to be. > > Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com> > --- > src/drivers/driver_nl80211.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index > 7e3de51..f256655 100644 > --- a/src/drivers/driver_nl80211.c > +++ b/src/drivers/driver_nl80211.c > @@ -4673,6 +4673,7 @@ wpa_driver_nl80211_finish_drv_init(struct > wpa_driver_nl80211_data *drv, > struct i802_bss *bss = drv->first_bss; > int send_rfkill_event = 0; > enum nl80211_iftype nlmode; > + int ret; > > drv->ifindex = if_nametoindex(bss->ifname); > bss->ifindex = drv->ifindex; > @@ -4710,25 +4711,26 @@ wpa_driver_nl80211_finish_drv_init(struct > wpa_driver_nl80211_data *drv, > return -1; > } > > - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) > - nl80211_get_macaddr(bss); > - > - if (!rfkill_is_blocked(drv->rfkill)) { > - int ret = i802_set_iface_flags(bss, 1); > - if (ret) { > - wpa_printf(MSG_ERROR, "nl80211: Could not set " > - "interface '%s' UP", bss->ifname); > - return ret; > + ret = i802_set_iface_flags(bss, 1); > + if (ret < 0) { > + if (rfkill_is_blocked(drv->rfkill)) { > + wpa_printf(MSG_DEBUG, "nl80211: Could not yet > enable " > + "interface '%s' due to rfkill", > + bss->ifname); > + drv->if_disabled = 1; > + send_rfkill_event = 1; > + } else { > + wpa_printf(MSG_ERROR, "nl80211: Could not %s " > + "interface '%s' UP", > + nlmode == NL80211_IFTYPE_P2P_DEVICE ? > + "start P2P" : "set", bss->ifname); > + return -1; Can use "return ret;" > } > - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) > - return ret; > - } else { > - wpa_printf(MSG_DEBUG, "nl80211: Could not yet enable " > - "interface '%s' due to rfkill", bss->ifname); > - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) > - return 0; > - drv->if_disabled = 1; > - send_rfkill_event = 1; > + } > + > + if (nlmode == NL80211_IFTYPE_P2P_DEVICE) { > + nl80211_get_macaddr(bss); > + return ret; Returning here is problematic as the wpa_driver_nl80211_send_rfkill() will not be called, and the INTERFACE_DISABLED event will not be fired. Regards, Ilan.
Hi Ilan, >> + } else { >> + wpa_printf(MSG_ERROR, "nl80211: Could not %s " >> + "interface '%s' UP", >> + nlmode == NL80211_IFTYPE_P2P_DEVICE ? >> + "start P2P" : "set", bss->ifname); >> + return -1; > Can use "return ret;" No need to, actually there is only one time the return code of wpa_driver_nl80211_finish_drv_init() is checked to be different than 0 and that's it. The ret variable is only used for the part below on which you have a comment. > >> } >> - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) >> - return ret; >> - } else { >> - wpa_printf(MSG_DEBUG, "nl80211: Could not yet enable " >> - "interface '%s' due to rfkill", bss->ifname); >> - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) >> - return 0; >> - drv->if_disabled = 1; >> - send_rfkill_event = 1; >> + } >> + >> + if (nlmode == NL80211_IFTYPE_P2P_DEVICE) { >> + nl80211_get_macaddr(bss); >> + return ret; > Returning here is problematic as the wpa_driver_nl80211_send_rfkill() will not be called, and the INTERFACE_DISABLED event will not be fired. > That logic was present before my patch and before Moshe's one. So you mean we can continue here? Then there would be no need of ret variable at all. Tomasz
Hi Tomasz, > >> + } else { > >> + wpa_printf(MSG_ERROR, "nl80211: Could not %s " > >> + "interface '%s' UP", > >> + nlmode == NL80211_IFTYPE_P2P_DEVICE ? > >> + "start P2P" : "set", bss->ifname); > >> + return -1; > > Can use "return ret;" > > No need to, actually there is only one time the return code of > wpa_driver_nl80211_finish_drv_init() is checked to be different than 0 and > that's it. The ret variable is only used for the part below on which you have a > comment. > Ok. > >> + if (nlmode == NL80211_IFTYPE_P2P_DEVICE) { > >> + nl80211_get_macaddr(bss); > >> + return ret; > > Returning here is problematic as the wpa_driver_nl80211_send_rfkill() will > not be called, and the INTERFACE_DISABLED event will not be fired. > > > > That logic was present before my patch and before Moshe's one. > > So you mean we can continue here? Then there would be no need of ret > variable at all. > I mean that for this to work for P2P_DEVICE as well, the following logic needs to be called, otherwise, the wpa_s will not handle rfkill off properly. if (send_rfkill_event) { eloop_register_timeout(0, 0, wpa_driver_nl80211_send_rfkill, drv, drv->ctx); } Regards, Ilan.
Hi Ilan, >>>> + if (nlmode == NL80211_IFTYPE_P2P_DEVICE) { >>>> > >>+ nl80211_get_macaddr(bss); >>>> > >>+ return ret; >>> > >Returning here is problematic as the wpa_driver_nl80211_send_rfkill() will >> >not be called, and the INTERFACE_DISABLED event will not be fired. >>> > > >> > >> >That logic was present before my patch and before Moshe's one. >> > >> >So you mean we can continue here? Then there would be no need of ret >> >variable at all. >> > > I mean that for this to work for P2P_DEVICE as well, the following logic needs to be called, otherwise, the wpa_s will not handle rfkill off properly. > > if (send_rfkill_event) { > eloop_register_timeout(0, 0, wpa_driver_nl80211_send_rfkill, > drv, drv->ctx); > } Indeed, I will prepare another patch then. Tomasz
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index 7e3de51..f256655 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -4673,6 +4673,7 @@ wpa_driver_nl80211_finish_drv_init(struct wpa_driver_nl80211_data *drv, struct i802_bss *bss = drv->first_bss; int send_rfkill_event = 0; enum nl80211_iftype nlmode; + int ret; drv->ifindex = if_nametoindex(bss->ifname); bss->ifindex = drv->ifindex; @@ -4710,25 +4711,26 @@ wpa_driver_nl80211_finish_drv_init(struct wpa_driver_nl80211_data *drv, return -1; } - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) - nl80211_get_macaddr(bss); - - if (!rfkill_is_blocked(drv->rfkill)) { - int ret = i802_set_iface_flags(bss, 1); - if (ret) { - wpa_printf(MSG_ERROR, "nl80211: Could not set " - "interface '%s' UP", bss->ifname); - return ret; + ret = i802_set_iface_flags(bss, 1); + if (ret < 0) { + if (rfkill_is_blocked(drv->rfkill)) { + wpa_printf(MSG_DEBUG, "nl80211: Could not yet enable " + "interface '%s' due to rfkill", + bss->ifname); + drv->if_disabled = 1; + send_rfkill_event = 1; + } else { + wpa_printf(MSG_ERROR, "nl80211: Could not %s " + "interface '%s' UP", + nlmode == NL80211_IFTYPE_P2P_DEVICE ? + "start P2P" : "set", bss->ifname); + return -1; } - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) - return ret; - } else { - wpa_printf(MSG_DEBUG, "nl80211: Could not yet enable " - "interface '%s' due to rfkill", bss->ifname); - if (nlmode == NL80211_IFTYPE_P2P_DEVICE) - return 0; - drv->if_disabled = 1; - send_rfkill_event = 1; + } + + if (nlmode == NL80211_IFTYPE_P2P_DEVICE) { + nl80211_get_macaddr(bss); + return ret; } if (!drv->hostapd)
This fixes a regression which is assuming that since the general rf-kill status is on, all devices are rf-killed. This rule is actually not true since this rf-kill status is not per-device specific and the actual device might not be rf-killed though users are prevented to use it due to that logic. The kernel knows better which device is rf-killed or not, and will check its status properly when it tries to set it up (or "start" it for P2P). Thus it's better to rely on the kernel's returned code as it used to be. Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com> --- src/drivers/driver_nl80211.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-)