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 <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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;
--
2.39.3
On 2/8/2024 9:51 AM, Alexey Berezhok 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.
Thanks, Alexey
Makes sense, but ....
> Signed-off-by: Alexey Berezhok <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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);
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?
@Johannes:
Actually not quite understanding the reason for having this setting in
nl80211. hidden_ssid means SSID element length is zero, right?
Regards,
Arend
> 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;
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
Patch applied to wireless-next.git, thanks.
f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value to boolean
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2/12/2024 4:38 PM, Kalle Valo wrote:
> Alexey Berezhok <[email protected]> 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 <[email protected]>
>
> 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/[email protected]/
Arend van Spriel <[email protected]> writes:
> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>> Alexey Berezhok <[email protected]> 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 <[email protected]>
>> 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?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On February 12, 2024 5:03:14 PM Kalle Valo <[email protected]> wrote:
> Arend van Spriel <[email protected]> writes:
>
>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>> Alexey Berezhok <[email protected]> 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 <[email protected]>
>>> 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 <[email protected]> writes:
> On February 12, 2024 5:03:14 PM Kalle Valo <[email protected]> wrote:
>
>> Arend van Spriel <[email protected]> writes:
>>
>>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>>> Alexey Berezhok <[email protected]> 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 <[email protected]>
>>>> 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 :)
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2/13/2024 2:34 PM, Kalle Valo wrote:
> Arend Van Spriel <[email protected]> writes:
>
>> On February 12, 2024 5:03:14 PM Kalle Valo <[email protected]> wrote:
>>
>>> Arend van Spriel <[email protected]> writes:
>>>
>>>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>>>> Alexey Berezhok <[email protected]> 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 <[email protected]>
>>>>> 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 <[email protected]> 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 :)
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On February 13, 2024 4:46:38 PM Kalle Valo <[email protected]> wrote:
> Arend van Spriel <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>
>> 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/[email protected]/
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 <[email protected]> wrote:
> 12.02.2024 19:00, Arend van Spriel wrote:
>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>> Alexey Berezhok <[email protected]> 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 <[email protected]>
>>>
>>> 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/[email protected]/
>
> 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)
--
With Best Regards,
Andy Shevchenko