| Message ID | 20240208085121.2430-1-a@bayrepo.ru |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: brcmfmac: do not cast hidden SSID attribute value to boolean | expand |
On Thu, 2024-02-08 at 15:06 +0100, Arend van Spriel wrote: > > settings->hidden_ssid has already been used above in following call: > > err = brcmf_fil_iovar_int_set(ifp, "closednet", > settings->hidden_ssid); > > So we pass the value as is to firmware using the same assumption, ie. > NL80211_HIDDEN_SSID_NOT_IN_USE. Is this not ABI thus very unlikely to > change? The ABI won't change, that'd break all the users of nl80211 that use this :-) > @Johannes: > Actually not quite understanding the reason for having this setting in > nl80211. hidden_ssid means SSID element length is zero, right? > Well, there at least _were_ APs doing the correct SSID length but setting all octets to zero ... Not sure that's still a thing though. johannes
On February 8, 2024 3:15:25 PM Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2024-02-08 at 15:06 +0100, Arend van Spriel wrote: >> >> settings->hidden_ssid has already been used above in following call: >> >> err = brcmf_fil_iovar_int_set(ifp, "closednet", >> settings->hidden_ssid); >> >> So we pass the value as is to firmware using the same assumption, ie. >> NL80211_HIDDEN_SSID_NOT_IN_USE. Is this not ABI thus very unlikely to >> change? > > The ABI won't change, that'd break all the users of nl80211 that use > this :-) Right. So basically the assumption that NL80211_HIDDEN_SSID_NOT_IN_USE is and will be zero is a safe one. > >> @Johannes: >> Actually not quite understanding the reason for having this setting in >> nl80211. hidden_ssid means SSID element length is zero, right? > > Well, there at least _were_ APs doing the correct SSID length but > setting all octets to zero ... Not sure that's still a thing though. I now looked at the definition and see indeed two distinct flavours of "hidden SSID". Interesting although I have never considered it a useful feature. Regards, Arend
Alexey Berezhok <a@bayrepo.ru> wrote: > In 'brcmf_cfg80211_start_ap()', not assume that > NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer > an explicit check instead. Compile tested only. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Alexey Berezhok <a@bayrepo.ru> Patch applied to wireless-next.git, thanks. f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value to boolean
On 2/12/2024 4:38 PM, Kalle Valo wrote: > Alexey Berezhok <a@bayrepo.ru> wrote: > >> In 'brcmf_cfg80211_start_ap()', not assume that >> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer >> an explicit check instead. Compile tested only. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Signed-off-by: Alexey Berezhok <a@bayrepo.ru> > > Patch applied to wireless-next.git, thanks. > > f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value to boolean Alexey, Can you do a follow-up patch addressing my comment? If not I will do it myself. Regards, Arend -- https://patchwork.kernel.org/project/linux-wireless/patch/20240208085121.2430-1-a@bayrepo.ru/
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > On 2/12/2024 4:38 PM, Kalle Valo wrote: >> Alexey Berezhok <a@bayrepo.ru> wrote: >> >>> In 'brcmf_cfg80211_start_ap()', not assume that >>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer >>> an explicit check instead. Compile tested only. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Signed-off-by: Alexey Berezhok <a@bayrepo.ru> >> Patch applied to wireless-next.git, thanks. >> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value >> to boolean > > Alexey, > > Can you do a follow-up patch addressing my comment? If not I will do > it myself. Sorry, was I not supposed to apply the patch? What did I miss?
On February 12, 2024 5:03:14 PM Kalle Valo <kvalo@kernel.org> wrote: > Arend van Spriel <arend.vanspriel@broadcom.com> writes: > >> On 2/12/2024 4:38 PM, Kalle Valo wrote: >>> Alexey Berezhok <a@bayrepo.ru> wrote: >>> >>>> In 'brcmf_cfg80211_start_ap()', not assume that >>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer >>>> an explicit check instead. Compile tested only. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>> >>>> Signed-off-by: Alexey Berezhok <a@bayrepo.ru> >>> Patch applied to wireless-next.git, thanks. >>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value >>> to boolean >> >> Alexey, >> >> Can you do a follow-up patch addressing my comment? If not I will do >> it myself. > > Sorry, was I not supposed to apply the patch? What did I miss? Nothing serious. settings->hidden_ssid value is used as-is to configure firmware. I wanted Alexey to address that in a v2. Regards, Arend > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > On February 12, 2024 5:03:14 PM Kalle Valo <kvalo@kernel.org> wrote: > >> Arend van Spriel <arend.vanspriel@broadcom.com> writes: >> >>> On 2/12/2024 4:38 PM, Kalle Valo wrote: >>>> Alexey Berezhok <a@bayrepo.ru> wrote: >>>> >>>>> In 'brcmf_cfg80211_start_ap()', not assume that >>>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer >>>>> an explicit check instead. Compile tested only. >>>>> >>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>>> >>>>> Signed-off-by: Alexey Berezhok <a@bayrepo.ru> >>>> Patch applied to wireless-next.git, thanks. >>>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value >>>> to boolean >>> >>> Alexey, >>> >>> Can you do a follow-up patch addressing my comment? If not I will do >>> it myself. >> >> Sorry, was I not supposed to apply the patch? What did I miss? > > Nothing serious. settings->hidden_ssid value is used as-is to > configure firmware. I wanted Alexey to address that in a v2. My bad, I misunderstood your intentions. Luckily this time it wasn't serious. BTW to make super clear to me I would prefer that you (Arend) use Acked-by. It shows up in my script like the number '1' here: *[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - - 2 5d Colin Ian Ki Under Review So if I don't see your Acked-by then I will not even look at the patch :)
On 2/13/2024 2:34 PM, Kalle Valo wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > >> On February 12, 2024 5:03:14 PM Kalle Valo <kvalo@kernel.org> wrote: >> >>> Arend van Spriel <arend.vanspriel@broadcom.com> writes: >>> >>>> On 2/12/2024 4:38 PM, Kalle Valo wrote: >>>>> Alexey Berezhok <a@bayrepo.ru> wrote: >>>>> >>>>>> In 'brcmf_cfg80211_start_ap()', not assume that >>>>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer >>>>>> an explicit check instead. Compile tested only. >>>>>> >>>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>>>> >>>>>> Signed-off-by: Alexey Berezhok <a@bayrepo.ru> >>>>> Patch applied to wireless-next.git, thanks. >>>>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value >>>>> to boolean >>>> >>>> Alexey, >>>> >>>> Can you do a follow-up patch addressing my comment? If not I will do >>>> it myself. >>> >>> Sorry, was I not supposed to apply the patch? What did I miss? >> >> Nothing serious. settings->hidden_ssid value is used as-is to >> configure firmware. I wanted Alexey to address that in a v2. > > My bad, I misunderstood your intentions. Luckily this time it wasn't > serious. > > BTW to make super clear to me I would prefer that you (Arend) use > Acked-by. It shows up in my script like the number '1' here: > > *[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - - 2 5d Colin Ian Ki Under Review > > So if I don't see your Acked-by then I will not even look at the patch :) Sure. I tend to use Acked-by if things look sane a quick glance. If I need to dig further I prefer to use Reviewed-by. If I have comments to revise the patch I will refrain using them. Is that ok or you really want it to be Acked-by? Regards, Arend
Arend van Spriel <arend.vanspriel@broadcom.com> writes: >> My bad, I misunderstood your intentions. Luckily this time it wasn't >> serious. >> BTW to make super clear to me I would prefer that you (Arend) use >> Acked-by. It shows up in my script like the number '1' here: >> *[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - - >> 2 5d Colin Ian Ki Under Review >> So if I don't see your Acked-by then I will not even look at the >> patch :) > > Sure. I tend to use Acked-by if things look sane a quick glance. If I > need to dig further I prefer to use Reviewed-by. If I have comments to > revise the patch I will refrain using them. Is that ok or you really > want it to be Acked-by? Ah, now I understand better. My understanding is that the maintainer of the driver uses Acked-by and others use Reviewed-by. This says the same: https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by And yes, I would prefer if you could use Acked-by always. It's up to your judgement if you do just a peek or in-depth review :)
On February 13, 2024 4:46:38 PM Kalle Valo <kvalo@kernel.org> wrote: > Arend van Spriel <arend.vanspriel@broadcom.com> writes: > >>> My bad, I misunderstood your intentions. Luckily this time it wasn't >>> serious. >>> BTW to make super clear to me I would prefer that you (Arend) use >>> Acked-by. It shows up in my script like the number '1' here: >>> *[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - - >>> 2 5d Colin Ian Ki Under Review >>> So if I don't see your Acked-by then I will not even look at the >>> patch :) >> >> Sure. I tend to use Acked-by if things look sane a quick glance. If I >> need to dig further I prefer to use Reviewed-by. If I have comments to >> revise the patch I will refrain using them. Is that ok or you really >> want it to be Acked-by? > > Ah, now I understand better. My understanding is that the maintainer of > the driver uses Acked-by and others use Reviewed-by. This says the same: > > https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by > > And yes, I would prefer if you could use Acked-by always. It's up to > your judgement if you do just a peek or in-depth review :) No problem. I think I can do that. Regards, Arend
12.02.2024 19:00, Arend van Spriel wrote: > On 2/12/2024 4:38 PM, Kalle Valo wrote: >> Alexey Berezhok <a@bayrepo.ru> wrote: >> >>> In 'brcmf_cfg80211_start_ap()', not assume that >>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer >>> an explicit check instead. Compile tested only. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Signed-off-by: Alexey Berezhok <a@bayrepo.ru> >> >> Patch applied to wireless-next.git, thanks. >> >> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value >> to boolean > > Alexey, > > Can you do a follow-up patch addressing my comment? If not I will do it > myself. > > Regards, > Arend > -- > https://patchwork.kernel.org/project/linux-wireless/patch/20240208085121.2430-1-a@bayrepo.ru/ Hello, do you mean this kind of modification: diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index 63f6e9436..d8f7bd5ce 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -5269,7 +5269,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, } err = brcmf_fil_iovar_int_set(ifp, "closednet", - settings->hidden_ssid); + (settings->hidden_ssid == NL80211_HIDDEN_SSID_NOT_IN_USE? 0 : 1)); if (err) { bphy_err(drvr, "%s closednet error (%d)\n", (settings->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE) ? If so, I will make additional patch. Regards, Alexey
On February 14, 2024 11:28:28 AM Alexey Berezhok <a@bayrepo.ru> wrote: > 12.02.2024 19:00, Arend van Spriel wrote: >> On 2/12/2024 4:38 PM, Kalle Valo wrote: >>> Alexey Berezhok <a@bayrepo.ru> wrote: >>> >>>> In 'brcmf_cfg80211_start_ap()', not assume that >>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer >>>> an explicit check instead. Compile tested only. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>> >>>> Signed-off-by: Alexey Berezhok <a@bayrepo.ru> >>> >>> Patch applied to wireless-next.git, thanks. >>> >>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value >>> to boolean >> >> Alexey, >> >> Can you do a follow-up patch addressing my comment? If not I will do it >> myself. >> >> Regards, >> Arend >> -- >> https://patchwork.kernel.org/project/linux-wireless/patch/20240208085121.2430-1-a@bayrepo.ru/ > > Hello, do you mean this kind of modification: > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > index 63f6e9436..d8f7bd5ce 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > @@ -5269,7 +5269,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, > struct net_device *ndev, > } > > err = brcmf_fil_iovar_int_set(ifp, "closednet", > - settings->hidden_ssid); > + (settings->hidden_ssid == > NL80211_HIDDEN_SSID_NOT_IN_USE? 0 : 1)); > if (err) { > bphy_err(drvr, "%s closednet error (%d)\n", > (settings->hidden_ssid != > NL80211_HIDDEN_SSID_NOT_IN_USE) ? > > If so, I will make additional patch. That was indeed what I meant. Consider using local variable 'closednet' to pass in function call and use for error message. Regards, Arend > Regards, > Alexey
Thu, Feb 08, 2024 at 11:51:21AM +0300, Alexey Berezhok kirjoitti: ... > - settings->hidden_ssid ? > + (settings->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE) ? > "enabled" : "disabled", Can somebody switch this to use str_enabled_disabled() from string_choices.h? Ideally, add str_disabled_enabled() in that header (as macro with negation on its argument) and use here as str_disabled_enabled(settings->hidden_ssid == NL80211_HIDDEN_SSID_NOT_IN_USE)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index 736b2ada6..63f6e9436 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -5272,7 +5272,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, settings->hidden_ssid); if (err) { bphy_err(drvr, "%s closednet error (%d)\n", - settings->hidden_ssid ? + (settings->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE) ? "enabled" : "disabled", err); goto exit;
In 'brcmf_cfg80211_start_ap()', not assume that NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer an explicit check instead. Compile tested only. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Alexey Berezhok <a@bayrepo.ru> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)