diff mbox series

scan: limit COLOCATED_6GHZ flag to 6ghz devices

Message ID 20240103125638.243820-1-prestwoj@gmail.com (mailing list archive)
State New
Headers show
Series scan: limit COLOCATED_6GHZ flag to 6ghz devices | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
prestwoj/iwd-alpine-ci-fetch success Fetch PR
prestwoj/iwd-ci-gitlint success GitLint
prestwoj/iwd-ci-fetch success Fetch PR
prestwoj/iwd-ci-makedistcheck success Make Distcheck
prestwoj/iwd-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-ci-build success Build - Configure
prestwoj/iwd-alpine-ci-makedistcheck success Make Distcheck
prestwoj/iwd-alpine-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-alpine-ci-build success Build - Configure
prestwoj/iwd-ci-makecheckvalgrind success Make Check w/Valgrind
prestwoj/iwd-ci-makecheck success Make Check
prestwoj/iwd-ci-clang success clang PASS
prestwoj/iwd-alpine-ci-makecheck pending makecheck SKIP
prestwoj/iwd-alpine-ci-makecheckvalgrind fail Make FAIL: monitor/main.c: In function 'open_packet': monitor/main.c:176:17: error: implicit declaration of function 'close'; did you mean 'pclose'? [-Werror=implicit-function-declaration] 176 | close(fd); | ^~~~~ | pclose cc1: all warnings being treated as errors make[1]: *** [Makefile:2572: monitor/main.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1735: all] Error 2
prestwoj/iwd-ci-testrunner success test-runner PASS

Commit Message

James Prestwood Jan. 3, 2024, 12:56 p.m. UTC
It was seen that this flag seems to cause issues when in AP mode on
brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a
scan is issued clients will disconnect. After testing this behavior
was isolated to the use of the COLOCATED_6GHZ flag.

Besides working around the problem on this specific hardware the
patch itself makes sense as a non-6GHz capable device shouldn't use
this flag anyways.

As stated in the patch comment, this isn't really a catch all
workaround since the flag is still used for devices supporting 6GHz.
If additional hardware exhibits this behavior we may need additional
changes like a hardware blacklist or an explicit option to disable
the flag.

Reported-By: Keith G <ys3al35l@gmail.com>
---
 src/scan.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

James Prestwood Jan. 3, 2024, 12:58 p.m. UTC | #1
Hi Keith,

On 1/3/24 4:56 AM, James Prestwood wrote:
> It was seen that this flag seems to cause issues when in AP mode on
> brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a
> scan is issued clients will disconnect. After testing this behavior
> was isolated to the use of the COLOCATED_6GHZ flag.
>
> Besides working around the problem on this specific hardware the
> patch itself makes sense as a non-6GHz capable device shouldn't use
> this flag anyways.
>
> As stated in the patch comment, this isn't really a catch all
> workaround since the flag is still used for devices supporting 6GHz.
> If additional hardware exhibits this behavior we may need additional
> changes like a hardware blacklist or an explicit option to disable
> the flag.
>
> Reported-By: Keith G <ys3al35l@gmail.com>

Could you double check it still fixes the issue you were seeing on brcmfmac?

Thanks,

James

> ---
>   src/scan.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/src/scan.c b/src/scan.c
> index f48ffdef..8c6fdc08 100644
> --- a/src/scan.c
> +++ b/src/scan.c
> @@ -394,7 +394,20 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
>   	if (params->ap_scan)
>   		flags |= NL80211_SCAN_FLAG_AP;
>   
> -	flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
> +	/*
> +	 * TODO: This flag appears to cause some undesired behavior on brcmfmac
> +	 *       when the device is in AP mode, or has a secondary AP interface
> +	 *       running, causing clients to disconnect when a scan is issued.
> +	 *
> +	 *       Only using this flag for 6GHz capable devices will limit this
> +	 *       behavior to only 6GHz devices and in reality makes sense
> +	 *       because a non-6GHz device shouldn't use this flag anyways. If
> +	 *       more issues still are seen related to this we may need an
> +	 *       explicit workaround, either brcmfmac-specific or a disable
> +	 *       option.
> +	 */
> +	if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
> +		flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>   
>   	if (flags)
>   		l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
KeithG Jan. 3, 2024, 2:57 p.m. UTC | #2
On Wed, Jan 3, 2024 at 6:58 AM James Prestwood <prestwoj@gmail.com> wrote:
>
> Hi Keith,
>
> On 1/3/24 4:56 AM, James Prestwood wrote:
> > It was seen that this flag seems to cause issues when in AP mode on
> > brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a
> > scan is issued clients will disconnect. After testing this behavior
> > was isolated to the use of the COLOCATED_6GHZ flag.
> >
> > Besides working around the problem on this specific hardware the
> > patch itself makes sense as a non-6GHz capable device shouldn't use
> > this flag anyways.
> >
> > As stated in the patch comment, this isn't really a catch all
> > workaround since the flag is still used for devices supporting 6GHz.
> > If additional hardware exhibits this behavior we may need additional
> > changes like a hardware blacklist or an explicit option to disable
> > the flag.
> >
> > Reported-By: Keith G <ys3al35l@gmail.com>
>
> Could you double check it still fixes the issue you were seeing on brcmfmac?
>
> Thanks,
>
> James
>
> > ---
> >   src/scan.c | 15 ++++++++++++++-
> >   1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/scan.c b/src/scan.c
> > index f48ffdef..8c6fdc08 100644
> > --- a/src/scan.c
> > +++ b/src/scan.c
> > @@ -394,7 +394,20 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
> >       if (params->ap_scan)
> >               flags |= NL80211_SCAN_FLAG_AP;
> >
> > -     flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
> > +     /*
> > +      * TODO: This flag appears to cause some undesired behavior on brcmfmac
> > +      *       when the device is in AP mode, or has a secondary AP interface
> > +      *       running, causing clients to disconnect when a scan is issued.
> > +      *
> > +      *       Only using this flag for 6GHz capable devices will limit this
> > +      *       behavior to only 6GHz devices and in reality makes sense
> > +      *       because a non-6GHz device shouldn't use this flag anyways. If
> > +      *       more issues still are seen related to this we may need an
> > +      *       explicit workaround, either brcmfmac-specific or a disable
> > +      *       option.
> > +      */
> > +     if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
> > +             flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
> >
> >       if (flags)
> >               l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
James,

I will do this tonight. I was playing a lot with iwd that I had
patched with your suggestion last night. I was trying to resolve a
related problem and I did note that it was significantly more stable
with this patch, but I was still getting disconnected periodically. I
do not yet know if those disconnects were due to me or iwd and am
still investigating. I will continue this investigation with this new
patch applied. SHould this patch be against HEAD or can I safely patch
2.12?

Keith
James Prestwood Jan. 3, 2024, 3:01 p.m. UTC | #3
Hi Keith,

On 1/3/24 6:57 AM, KeithG wrote:
> On Wed, Jan 3, 2024 at 6:58 AM James Prestwood <prestwoj@gmail.com> wrote:
>> Hi Keith,
>>
>> On 1/3/24 4:56 AM, James Prestwood wrote:
>>> It was seen that this flag seems to cause issues when in AP mode on
>>> brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a
>>> scan is issued clients will disconnect. After testing this behavior
>>> was isolated to the use of the COLOCATED_6GHZ flag.
>>>
>>> Besides working around the problem on this specific hardware the
>>> patch itself makes sense as a non-6GHz capable device shouldn't use
>>> this flag anyways.
>>>
>>> As stated in the patch comment, this isn't really a catch all
>>> workaround since the flag is still used for devices supporting 6GHz.
>>> If additional hardware exhibits this behavior we may need additional
>>> changes like a hardware blacklist or an explicit option to disable
>>> the flag.
>>>
>>> Reported-By: Keith G <ys3al35l@gmail.com>
>> Could you double check it still fixes the issue you were seeing on brcmfmac?
>>
>> Thanks,
>>
>> James
>>
>>> ---
>>>    src/scan.c | 15 ++++++++++++++-
>>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/scan.c b/src/scan.c
>>> index f48ffdef..8c6fdc08 100644
>>> --- a/src/scan.c
>>> +++ b/src/scan.c
>>> @@ -394,7 +394,20 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
>>>        if (params->ap_scan)
>>>                flags |= NL80211_SCAN_FLAG_AP;
>>>
>>> -     flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>>> +     /*
>>> +      * TODO: This flag appears to cause some undesired behavior on brcmfmac
>>> +      *       when the device is in AP mode, or has a secondary AP interface
>>> +      *       running, causing clients to disconnect when a scan is issued.
>>> +      *
>>> +      *       Only using this flag for 6GHz capable devices will limit this
>>> +      *       behavior to only 6GHz devices and in reality makes sense
>>> +      *       because a non-6GHz device shouldn't use this flag anyways. If
>>> +      *       more issues still are seen related to this we may need an
>>> +      *       explicit workaround, either brcmfmac-specific or a disable
>>> +      *       option.
>>> +      */
>>> +     if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
>>> +             flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>>>
>>>        if (flags)
>>>                l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
> James,
>
> I will do this tonight. I was playing a lot with iwd that I had
> patched with your suggestion last night. I was trying to resolve a
> related problem and I did note that it was significantly more stable
> with this patch, but I was still getting disconnected periodically. I
> do not yet know if those disconnects were due to me or iwd and am
> still investigating. I will continue this investigation with this new
> patch applied. SHould this patch be against HEAD or can I safely patch
> 2.12?

Either works, there haven't been changes to that area in a while.

Thanks,

James

>
> Keith
KeithG Jan. 4, 2024, 2:42 a.m. UTC | #4
On Wed, Jan 3, 2024 at 9:01 AM James Prestwood <prestwoj@gmail.com> wrote:
>
> Hi Keith,
>
> On 1/3/24 6:57 AM, KeithG wrote:
> > On Wed, Jan 3, 2024 at 6:58 AM James Prestwood <prestwoj@gmail.com> wrote:
> >> Hi Keith,
> >>
> >> On 1/3/24 4:56 AM, James Prestwood wrote:
> >>> It was seen that this flag seems to cause issues when in AP mode on
> >>> brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a
> >>> scan is issued clients will disconnect. After testing this behavior
> >>> was isolated to the use of the COLOCATED_6GHZ flag.
> >>>
> >>> Besides working around the problem on this specific hardware the
> >>> patch itself makes sense as a non-6GHz capable device shouldn't use
> >>> this flag anyways.
> >>>
> >>> As stated in the patch comment, this isn't really a catch all
> >>> workaround since the flag is still used for devices supporting 6GHz.
> >>> If additional hardware exhibits this behavior we may need additional
> >>> changes like a hardware blacklist or an explicit option to disable
> >>> the flag.
> >>>
> >>> Reported-By: Keith G <ys3al35l@gmail.com>
> >> Could you double check it still fixes the issue you were seeing on brcmfmac?
> >>
> >> Thanks,
> >>
> >> James
> >>
> >>> ---
> >>>    src/scan.c | 15 ++++++++++++++-
> >>>    1 file changed, 14 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/scan.c b/src/scan.c
> >>> index f48ffdef..8c6fdc08 100644
> >>> --- a/src/scan.c
> >>> +++ b/src/scan.c
> >>> @@ -394,7 +394,20 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
> >>>        if (params->ap_scan)
> >>>                flags |= NL80211_SCAN_FLAG_AP;
> >>>
> >>> -     flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
> >>> +     /*
> >>> +      * TODO: This flag appears to cause some undesired behavior on brcmfmac
> >>> +      *       when the device is in AP mode, or has a secondary AP interface
> >>> +      *       running, causing clients to disconnect when a scan is issued.
> >>> +      *
> >>> +      *       Only using this flag for 6GHz capable devices will limit this
> >>> +      *       behavior to only 6GHz devices and in reality makes sense
> >>> +      *       because a non-6GHz device shouldn't use this flag anyways. If
> >>> +      *       more issues still are seen related to this we may need an
> >>> +      *       explicit workaround, either brcmfmac-specific or a disable
> >>> +      *       option.
> >>> +      */
> >>> +     if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
> >>> +             flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
> >>>
> >>>        if (flags)
> >>>                l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
> > James,
> >
> > I will do this tonight. I was playing a lot with iwd that I had
> > patched with your suggestion last night. I was trying to resolve a
> > related problem and I did note that it was significantly more stable
> > with this patch, but I was still getting disconnected periodically. I
> > do not yet know if those disconnects were due to me or iwd and am
> > still investigating. I will continue this investigation with this new
> > patch applied. SHould this patch be against HEAD or can I safely patch
> > 2.12?
>
> Either works, there haven't been changes to that area in a while.
>
> Thanks,
>
> James
>
> >
> > Keith
James,

I patched and am playing with the Pi3b+. It is still flaky. I connect
via hostapd (hostapd_3.log) and let the journal run. When I get a
disconnect, this is what the journal shows:

Jan 03 20:04:02 runeaudio iwd[3610]: src/station.c:process_network()
No remaining BSSs for SSID: ATT4DwP79U -- Removing network
Jan 03 20:04:02 runeaudio iwd[3610]: src/station.c:process_network()
No remaining BSSs for SSID: TP-LINK_64C5 -- Removing network
Jan 03 20:04:02 runeaudio iwd[3610]: src/station.c:process_network()
No remaining BSSs for SSID: BingoBango -- Removing network
Jan 03 20:04:02 runeaudio iwd[3610]: src/station.c:process_network()
No remaining BSSs for SSID: Butchie -- Removing network
Jan 03 20:04:02 runeaudio iwd[3610]: src/station.c:process_network()
No remaining BSSs for SSID: NETGEAR45 -- Removing network
Jan 03 20:04:02 runeaudio iwd[3610]:
src/wiphy.c:wiphy_radio_work_insert() Inserting work item 8
Jan 03 20:04:02 runeaudio iwd[3610]:
src/wiphy.c:wiphy_radio_work_done() Work item 7 done
Jan 03 20:04:02 runeaudio iwd[3610]:
src/wiphy.c:wiphy_radio_work_next() Starting work item 8
Jan 03 20:04:02 runeaudio iwd[3610]: src/scan.c:scan_notify() Scan
notification Trigger Scan(33)
Jan 03 20:04:02 runeaudio iwd[3610]:
src/scan.c:scan_request_triggered() Passive scan triggered for wdev 1
Jan 03 20:04:02 runeaudio iwd[3610]:
src/station.c:station_dbus_scan_triggered() station_scan_triggered: 0
Jan 03 20:04:02 runeaudio iwd[3610]:
src/station.c:station_dbus_scan_triggered() Scan triggered for wlan0
subset 1

tried again with iwd (iwd_3.log) and got this.
Jan 03 20:25:06 runeaudio iwd[1338]:
src/wiphy.c:wiphy_radio_work_done() Work item 20 done
Jan 03 20:26:04 runeaudio mpdscribble[2996]: [last.fm] waiting 240
seconds before trying again
Jan 03 20:27:46 runeaudio iwd[1338]: src/scan.c:scan_periodic_timeout() 1
Jan 03 20:27:46 runeaudio iwd[1338]:
src/wiphy.c:wiphy_radio_work_insert() Inserting work item 21
Jan 03 20:27:46 runeaudio iwd[1338]:
src/wiphy.c:wiphy_radio_work_next() Starting work item 21
Jan 03 20:27:46 runeaudio iwd[1338]: src/scan.c:scan_notify() Scan
notification Trigger Scan(33)
Jan 03 20:27:46 runeaudio iwd[1338]:
src/scan.c:scan_request_triggered() Passive scan triggered for wdev 1
Jan 03 20:27:46 runeaudio iwd[1338]:
src/scan.c:scan_periodic_triggered() Periodic scan triggered for wdev
1
James Prestwood Jan. 4, 2024, 12:54 p.m. UTC | #5
Hi Keith,

On 1/3/24 6:42 PM, KeithG wrote:
> On Wed, Jan 3, 2024 at 9:01 AM James Prestwood <prestwoj@gmail.com> wrote:
>> Hi Keith,
>>
>> On 1/3/24 6:57 AM, KeithG wrote:
>>> On Wed, Jan 3, 2024 at 6:58 AM James Prestwood <prestwoj@gmail.com> wrote:
>>>> Hi Keith,
>>>>
>>>> On 1/3/24 4:56 AM, James Prestwood wrote:
>>>>> It was seen that this flag seems to cause issues when in AP mode on
>>>>> brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a
>>>>> scan is issued clients will disconnect. After testing this behavior
>>>>> was isolated to the use of the COLOCATED_6GHZ flag.
>>>>>
>>>>> Besides working around the problem on this specific hardware the
>>>>> patch itself makes sense as a non-6GHz capable device shouldn't use
>>>>> this flag anyways.
>>>>>
>>>>> As stated in the patch comment, this isn't really a catch all
>>>>> workaround since the flag is still used for devices supporting 6GHz.
>>>>> If additional hardware exhibits this behavior we may need additional
>>>>> changes like a hardware blacklist or an explicit option to disable
>>>>> the flag.
>>>>>
>>>>> Reported-By: Keith G <ys3al35l@gmail.com>
>>>> Could you double check it still fixes the issue you were seeing on brcmfmac?
>>>>
>>>> Thanks,
>>>>
>>>> James
>>>>
>>>>> ---
>>>>>     src/scan.c | 15 ++++++++++++++-
>>>>>     1 file changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/scan.c b/src/scan.c
>>>>> index f48ffdef..8c6fdc08 100644
>>>>> --- a/src/scan.c
>>>>> +++ b/src/scan.c
>>>>> @@ -394,7 +394,20 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
>>>>>         if (params->ap_scan)
>>>>>                 flags |= NL80211_SCAN_FLAG_AP;
>>>>>
>>>>> -     flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>>>>> +     /*
>>>>> +      * TODO: This flag appears to cause some undesired behavior on brcmfmac
>>>>> +      *       when the device is in AP mode, or has a secondary AP interface
>>>>> +      *       running, causing clients to disconnect when a scan is issued.
>>>>> +      *
>>>>> +      *       Only using this flag for 6GHz capable devices will limit this
>>>>> +      *       behavior to only 6GHz devices and in reality makes sense
>>>>> +      *       because a non-6GHz device shouldn't use this flag anyways. If
>>>>> +      *       more issues still are seen related to this we may need an
>>>>> +      *       explicit workaround, either brcmfmac-specific or a disable
>>>>> +      *       option.
>>>>> +      */
>>>>> +     if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
>>>>> +             flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>>>>>
>>>>>         if (flags)
>>>>>                 l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
>>> James,
>>>
>>> I will do this tonight. I was playing a lot with iwd that I had
>>> patched with your suggestion last night. I was trying to resolve a
>>> related problem and I did note that it was significantly more stable
>>> with this patch, but I was still getting disconnected periodically. I
>>> do not yet know if those disconnects were due to me or iwd and am
>>> still investigating. I will continue this investigation with this new
>>> patch applied. SHould this patch be against HEAD or can I safely patch
>>> 2.12?

So here is what I found. If I have just a single interface, AP mode, I 
can connect and scan (in AP mode) just fine without resulting in a 
disconnect. If I even create a secondary interface I can no longer 
connect what-so-ever. I see the SSID on my client (Android) device and 
try and connect but see no attempt on the AP side of things. This even 
happens if the second interface is left down, which is really odd to me. 
Could you include exactly how you're setting things up? What interfaces 
you create, etc.

Whats odd is the patch I gave you to privately simply commented out that 
flag and you said that worked, maybe spoke too soon? This patch would 
have no functional difference since the raspi doesn't support 6GHz.

Thanks,

James
Denis Kenzior Jan. 4, 2024, 6:17 p.m. UTC | #6
Hi James,

On 1/3/24 06:56, James Prestwood wrote:
> It was seen that this flag seems to cause issues when in AP mode on
> brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a

Seriously?  Why would this flag do anything for non-6E devices?

> scan is issued clients will disconnect. After testing this behavior
> was isolated to the use of the COLOCATED_6GHZ flag.
> 
> Besides working around the problem on this specific hardware the
> patch itself makes sense as a non-6GHz capable device shouldn't use
> this flag anyways.

I guess if you're super paranoid and don't trust the kernel to do the right thing...

> 
> As stated in the patch comment, this isn't really a catch all
> workaround since the flag is still used for devices supporting 6GHz.
> If additional hardware exhibits this behavior we may need additional
> changes like a hardware blacklist or an explicit option to disable
> the flag.
> 
> Reported-By: Keith G <ys3al35l@gmail.com>
> ---
>   src/scan.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/scan.c b/src/scan.c
> index f48ffdef..8c6fdc08 100644
> --- a/src/scan.c
> +++ b/src/scan.c
> @@ -394,7 +394,20 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
>   	if (params->ap_scan)
>   		flags |= NL80211_SCAN_FLAG_AP;
>   
> -	flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
> +	/*
> +	 * TODO: This flag appears to cause some undesired behavior on brcmfmac
> +	 *       when the device is in AP mode, or has a secondary AP interface
> +	 *       running, causing clients to disconnect when a scan is issued.
> +	 *
> +	 *       Only using this flag for 6GHz capable devices will limit this
> +	 *       behavior to only 6GHz devices and in reality makes sense
> +	 *       because a non-6GHz device shouldn't use this flag anyways. If
> +	 *       more issues still are seen related to this we may need an
> +	 *       explicit workaround, either brcmfmac-specific or a disable
> +	 *       option.
> +	 */

I'd just drop this comment entirely if you still think this is needed.

> +	if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
> +		flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>   
>   	if (flags)
>   		l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);

Regards,
-Denis
James Prestwood Jan. 4, 2024, 8:30 p.m. UTC | #7
Hi Denis,

On 1/4/24 10:17 AM, Denis Kenzior wrote:
> Hi James,
>
> On 1/3/24 06:56, James Prestwood wrote:
>> It was seen that this flag seems to cause issues when in AP mode on
>> brcmfmac devices (e.g. the raspberry Pi 3). When in AP mode an a
>
> Seriously?  Why would this flag do anything for non-6E devices?

No idea, I haven't had time, or the desire really to actually check the 
driver. But it was night and day in my testing. With the flag enabled a 
scan on the station interface would immediately disconnect all clients. 
Without the flag things were stable.

We can definitely hold off here, Keith is still doing some testing. Its 
not critical to anything I'm doing so I'd rather be sure it actually 
fixes the problem.

>
>> scan is issued clients will disconnect. After testing this behavior
>> was isolated to the use of the COLOCATED_6GHZ flag.
>>
>> Besides working around the problem on this specific hardware the
>> patch itself makes sense as a non-6GHz capable device shouldn't use
>> this flag anyways.
>
> I guess if you're super paranoid and don't trust the kernel to do the 
> right thing...
>
>>
>> As stated in the patch comment, this isn't really a catch all
>> workaround since the flag is still used for devices supporting 6GHz.
>> If additional hardware exhibits this behavior we may need additional
>> changes like a hardware blacklist or an explicit option to disable
>> the flag.
>>
>> Reported-By: Keith G <ys3al35l@gmail.com>
>> ---
>>   src/scan.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/scan.c b/src/scan.c
>> index f48ffdef..8c6fdc08 100644
>> --- a/src/scan.c
>> +++ b/src/scan.c
>> @@ -394,7 +394,20 @@ static struct l_genl_msg *scan_build_cmd(struct 
>> scan_context *sc,
>>       if (params->ap_scan)
>>           flags |= NL80211_SCAN_FLAG_AP;
>>   -    flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>> +    /*
>> +     * TODO: This flag appears to cause some undesired behavior on 
>> brcmfmac
>> +     *       when the device is in AP mode, or has a secondary AP 
>> interface
>> +     *       running, causing clients to disconnect when a scan is 
>> issued.
>> +     *
>> +     *       Only using this flag for 6GHz capable devices will 
>> limit this
>> +     *       behavior to only 6GHz devices and in reality makes sense
>> +     *       because a non-6GHz device shouldn't use this flag 
>> anyways. If
>> +     *       more issues still are seen related to this we may need an
>> +     *       explicit workaround, either brcmfmac-specific or a disable
>> +     *       option.
>> +     */
>
> I'd just drop this comment entirely if you still think this is needed.
>
>> +    if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
>> +        flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
>>         if (flags)
>>           l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, 
>> &flags);
>
> Regards,
> -Denis
diff mbox series

Patch

diff --git a/src/scan.c b/src/scan.c
index f48ffdef..8c6fdc08 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -394,7 +394,20 @@  static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 	if (params->ap_scan)
 		flags |= NL80211_SCAN_FLAG_AP;
 
-	flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
+	/*
+	 * TODO: This flag appears to cause some undesired behavior on brcmfmac
+	 *       when the device is in AP mode, or has a secondary AP interface
+	 *       running, causing clients to disconnect when a scan is issued.
+	 *
+	 *       Only using this flag for 6GHz capable devices will limit this
+	 *       behavior to only 6GHz devices and in reality makes sense
+	 *       because a non-6GHz device shouldn't use this flag anyways. If
+	 *       more issues still are seen related to this we may need an
+	 *       explicit workaround, either brcmfmac-specific or a disable
+	 *       option.
+	 */
+	if (wiphy_band_is_disabled(sc->wiphy, BAND_FREQ_6_GHZ) != -ENOTSUP)
+		flags |= NL80211_SCAN_FLAG_COLOCATED_6GHZ;
 
 	if (flags)
 		l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);