diff mbox

ath10k firmware sends probes on DFS channels without radar detection

Message ID 00f501d256e7$0f8d5380$2ea7fa80$@acksys.fr (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Jean-Pierre TOSONI Dec. 15, 2016, 3:22 p.m. UTC
Jouni,

Thanks for the suggestion, I already tried something like this in wmi.c,
with the same result:

- Before patching the firmware scans DFS channels actively (with probes).

- After patching, the firmware scans DFS channels passively *until* any
beacon is received on the DFS channel. When *any* beacon is seen, the
firmware decides to scan actively on its own, without any new IR/RADAR
info from the driver.

So, your patch is required but not sufficient.

Somehow I was able to overcome this by reloading the regulation domain
in the radio card before each scan request:

////// awful patch ahead ////////


////////////////////////////////////////

...But this sets a terrible penalty on performance when applied to
background scan.


On 12/14/16 20:58 Jouni Malinen wrote:
> 
> On Tue, Dec 06, 2016 at 06:02:52PM +0100, Jean-Pierre Tosoni wrote:
> > This follows on the previous discussion
> > 	"Client station sends probes on DFS channels"
> >
> > Problem:
> > The combination of QCA988X firmware v10.2.4.70-2 + ath10k +
> > wpa_supplicant do not comply with the norm ETSI/EN 301-893 section
> > 4.7; because they can send probes for 600s when no AP is around.
> >
> > Analysis:
> > The problem seems to lie in the firmware, which regards the presence
> > of *any* beacon as a proof that the channel is radar-clean for 600s.
> 
> I don't think this is really firmware, but cfg80211 regulatory code and
> how it interacts with ath10k..
> 
> > - there is no obvious fix working in ath10k.
> > - the issue does not show up with other mac80211 devices like ath9k.
> > - wpa_supplicant considers this is a kernel issue [2]
> 
> There seems to be a difference between ath9k (mac80211-based Probe Request
> frame sending) and ath10k (firmware) in this area for active scanning.
> mac80211 uses IEEE80211_CHAN_NO_IR | IEEE80211_CHAN_RADAR while ath10k
> uses IEEE80211_CHAN_NO_IR. I'd assume this difference results in ath10k
> using cfg80211 beacon hints (etc.) to update the NO_IR flag and that might
> be behind the difference you see.
> 
> Could you check whether the following change gets you the behavior you
> want to see here? I have not had a chance to test this yet, but based on
> code review, it looks like something that brings the same behavior to
> ath10k that ath9k has for this through mac80211.
> 
> 
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
> b/drivers/net/wireless/ath/ath10k/mac.c
> index aa545a1..758dbbd 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2973,7 +2973,8 @@ static int ath10k_update_channel_list(struct ath10k
> *ar)
>  			ch->chan_radar =
>  				!!(channel->flags & IEEE80211_CHAN_RADAR);
> 
> -			passive = channel->flags & IEEE80211_CHAN_NO_IR;
> +			passive = channel->flags & (IEEE80211_CHAN_NO_IR |
> +						    IEEE80211_CHAN_RADAR);
>  			ch->passive = passive;
> 
>  			ch->freq = channel->center_freq;
> 
> --
> Jouni Malinen                                            PGP id EFC895FA
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

Comments

Ben Greear Dec. 15, 2016, 4:32 p.m. UTC | #1
On 12/15/2016 07:22 AM, Jean-Pierre Tosoni wrote:
> Jouni,
>
> Thanks for the suggestion, I already tried something like this in wmi.c,
> with the same result:
>
> - Before patching the firmware scans DFS channels actively (with probes).
>
> - After patching, the firmware scans DFS channels passively *until* any
> beacon is received on the DFS channel. When *any* beacon is seen, the
> firmware decides to scan actively on its own, without any new IR/RADAR
> info from the driver.
>
> So, your patch is required but not sufficient.
>
> Somehow I was able to overcome this by reloading the regulation domain
> in the radio card before each scan request:
>
> ////// awful patch ahead ////////
>
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2842,7 +2842,9 @@ static int ath10k_update_channel_list(st
>   			ch->chan_radar =
>   				!!(channel->flags & IEEE80211_CHAN_RADAR);
>
> -			passive = channel->flags & IEEE80211_CHAN_NO_IR;
> +			passive = channel->flags & (IEEE80211_CHAN_NO_IR |
> +						    IEEE80211_CHAN_RADAR);

So, should we add a new flag in firmware and driver that means 'really-no-IR', or
should the NO_IR flag here just always make the firmware never do IR when probing
regardless of whether it has seen beacons or not?

Thanks,
Ben

> +
>   			ch->passive = passive;
>
>   			ch->freq = channel->center_freq;
> @@ -3548,6 +3550,9 @@ static int ath10k_start_scan(struct ath1
>
>   	lockdep_assert_held(&ar->conf_mutex);
>
> +	if (ar->state == ATH10K_STATE_ON)
> +		ath10k_regd_update(ar);
> +
>   	ret = ath10k_wmi_start_scan(ar, arg);
>   	if (ret)
>   		return ret;
>
> ////////////////////////////////////////
>
> ...But this sets a terrible penalty on performance when applied to
> background scan.
>
>
> On 12/14/16 20:58 Jouni Malinen wrote:
>>
>> On Tue, Dec 06, 2016 at 06:02:52PM +0100, Jean-Pierre Tosoni wrote:
>>> This follows on the previous discussion
>>> 	"Client station sends probes on DFS channels"
>>>
>>> Problem:
>>> The combination of QCA988X firmware v10.2.4.70-2 + ath10k +
>>> wpa_supplicant do not comply with the norm ETSI/EN 301-893 section
>>> 4.7; because they can send probes for 600s when no AP is around.
>>>
>>> Analysis:
>>> The problem seems to lie in the firmware, which regards the presence
>>> of *any* beacon as a proof that the channel is radar-clean for 600s.
>>
>> I don't think this is really firmware, but cfg80211 regulatory code and
>> how it interacts with ath10k..
>>
>>> - there is no obvious fix working in ath10k.
>>> - the issue does not show up with other mac80211 devices like ath9k.
>>> - wpa_supplicant considers this is a kernel issue [2]
>>
>> There seems to be a difference between ath9k (mac80211-based Probe Request
>> frame sending) and ath10k (firmware) in this area for active scanning.
>> mac80211 uses IEEE80211_CHAN_NO_IR | IEEE80211_CHAN_RADAR while ath10k
>> uses IEEE80211_CHAN_NO_IR. I'd assume this difference results in ath10k
>> using cfg80211 beacon hints (etc.) to update the NO_IR flag and that might
>> be behind the difference you see.
>>
>> Could you check whether the following change gets you the behavior you
>> want to see here? I have not had a chance to test this yet, but based on
>> code review, it looks like something that brings the same behavior to
>> ath10k that ath9k has for this through mac80211.
>>
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>> b/drivers/net/wireless/ath/ath10k/mac.c
>> index aa545a1..758dbbd 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -2973,7 +2973,8 @@ static int ath10k_update_channel_list(struct ath10k
>> *ar)
>>   			ch->chan_radar =
>>   				!!(channel->flags & IEEE80211_CHAN_RADAR);
>>
>> -			passive = channel->flags & IEEE80211_CHAN_NO_IR;
>> +			passive = channel->flags & (IEEE80211_CHAN_NO_IR |
>> +						    IEEE80211_CHAN_RADAR);
>>   			ch->passive = passive;
>>
>>   			ch->freq = channel->center_freq;
>>
>> --
>> Jouni Malinen                                            PGP id EFC895FA
>>
>> _______________________________________________
>> ath10k mailing list
>> ath10k@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/ath10k
>
Jean-Pierre TOSONI Dec. 15, 2016, 5:53 p.m. UTC | #2
> -----Message d'origine-----
> De : Ben Greear [mailto:greearb@candelatech.com]
> Envoyé : jeudi 15 décembre 2016 17:33
> À : Jean-Pierre Tosoni; 'Jouni Malinen'
> Cc : linux-wireless@vger.kernel.org; ath10k@lists.infradead.org
> Objet : Re: ath10k firmware sends probes on DFS channels without radar
> detection
> 
> On 12/15/2016 07:22 AM, Jean-Pierre Tosoni wrote:
> > Jouni,
> >
> > Thanks for the suggestion, I already tried something like this in
> > wmi.c, with the same result:
> >
> > - Before patching the firmware scans DFS channels actively (with
> probes).
> >
> > - After patching, the firmware scans DFS channels passively *until*
> > any beacon is received on the DFS channel. When *any* beacon is seen,
> > the firmware decides to scan actively on its own, without any new
> > IR/RADAR info from the driver.
> >
> > So, your patch is required but not sufficient.
> >
> > Somehow I was able to overcome this by reloading the regulation domain
> > in the radio card before each scan request:
> >
> > ////// awful patch ahead ////////
> >
> > --- a/drivers/net/wireless/ath/ath10k/mac.c
> > +++ b/drivers/net/wireless/ath/ath10k/mac.c
> > @@ -2842,7 +2842,9 @@ static int ath10k_update_channel_list(st
> >   			ch->chan_radar =
> >   				!!(channel->flags & IEEE80211_CHAN_RADAR);
> >
> > -			passive = channel->flags & IEEE80211_CHAN_NO_IR;
> > +			passive = channel->flags & (IEEE80211_CHAN_NO_IR |
> > +						    IEEE80211_CHAN_RADAR);
> 
> So, should we add a new flag in firmware and driver that means 'really-no-
> IR', or should the NO_IR flag here just always make the firmware never do
> IR when probing regardless of whether it has seen beacons or not?

The distinction between NO_IR and CHAN_RADAR is not very clear to me.
NO_IR appears only in the world regulatory domain so it's not relevant here.

I'd say
 "the CHAN_RADAR flag should always make the firmware never do IR when
probing"
...maybe, except if the channel is the operating channel. (this should
exclude
unassociated stations)

I am out of office for the next week.
Regards,
Jean-Pierre
> 
> Thanks,
> Ben
> 
> > +
> >   			ch->passive = passive;
> >
> >   			ch->freq = channel->center_freq;
> > @@ -3548,6 +3550,9 @@ static int ath10k_start_scan(struct ath1
> >
> >   	lockdep_assert_held(&ar->conf_mutex);
> >
> > +	if (ar->state == ATH10K_STATE_ON)
> > +		ath10k_regd_update(ar);
> > +
> >   	ret = ath10k_wmi_start_scan(ar, arg);
> >   	if (ret)
> >   		return ret;
> >
> > ////////////////////////////////////////
> >
> > ...But this sets a terrible penalty on performance when applied to
> > background scan.
> >
> >
> > On 12/14/16 20:58 Jouni Malinen wrote:
> >>
> >> On Tue, Dec 06, 2016 at 06:02:52PM +0100, Jean-Pierre Tosoni wrote:
> >>> This follows on the previous discussion
> >>> 	"Client station sends probes on DFS channels"
> >>>
> >>> Problem:
> >>> The combination of QCA988X firmware v10.2.4.70-2 + ath10k +
> >>> wpa_supplicant do not comply with the norm ETSI/EN 301-893 section
> >>> 4.7; because they can send probes for 600s when no AP is around.
> >>>
> >>> Analysis:
> >>> The problem seems to lie in the firmware, which regards the presence
> >>> of *any* beacon as a proof that the channel is radar-clean for 600s.
> >>
> >> I don't think this is really firmware, but cfg80211 regulatory code
> >> and how it interacts with ath10k..
> >>
> >>> - there is no obvious fix working in ath10k.
> >>> - the issue does not show up with other mac80211 devices like ath9k.
> >>> - wpa_supplicant considers this is a kernel issue [2]
> >>
> >> There seems to be a difference between ath9k (mac80211-based Probe
> >> Request frame sending) and ath10k (firmware) in this area for active
> scanning.
> >> mac80211 uses IEEE80211_CHAN_NO_IR | IEEE80211_CHAN_RADAR while
> >> ath10k uses IEEE80211_CHAN_NO_IR. I'd assume this difference results
> >> in ath10k using cfg80211 beacon hints (etc.) to update the NO_IR flag
> >> and that might be behind the difference you see.
> >>
> >> Could you check whether the following change gets you the behavior
> >> you want to see here? I have not had a chance to test this yet, but
> >> based on code review, it looks like something that brings the same
> >> behavior to ath10k that ath9k has for this through mac80211.
> >>
> >>
> >> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
> >> b/drivers/net/wireless/ath/ath10k/mac.c
> >> index aa545a1..758dbbd 100644
> >> --- a/drivers/net/wireless/ath/ath10k/mac.c
> >> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> >> @@ -2973,7 +2973,8 @@ static int ath10k_update_channel_list(struct
> >> ath10k
> >> *ar)
> >>   			ch->chan_radar =
> >>   				!!(channel->flags & IEEE80211_CHAN_RADAR);
> >>
> >> -			passive = channel->flags & IEEE80211_CHAN_NO_IR;
> >> +			passive = channel->flags & (IEEE80211_CHAN_NO_IR |
> >> +						    IEEE80211_CHAN_RADAR);
> >>   			ch->passive = passive;
> >>
> >>   			ch->freq = channel->center_freq;
> >>
> >> --
> >> Jouni Malinen                                            PGP id
> EFC895FA
> >>
> >> _______________________________________________
> >> ath10k mailing list
> >> ath10k@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/ath10k
> >
> 
> 
> --
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com
Jouni Malinen Dec. 15, 2016, 10:58 p.m. UTC | #3
On Thu, Dec 15, 2016 at 06:53:47PM +0100, Jean-Pierre Tosoni wrote:
> > > Thanks for the suggestion, I already tried something like this in
> > > wmi.c, with the same result:
> > >
> > > - Before patching the firmware scans DFS channels actively (with
> > probes).
> > >
> > > - After patching, the firmware scans DFS channels passively *until*
> > > any beacon is received on the DFS channel. When *any* beacon is seen,
> > > the firmware decides to scan actively on its own, without any new
> > > IR/RADAR info from the driver.
> > >
> > > So, your patch is required but not sufficient.
> > >
> > > Somehow I was able to overcome this by reloading the regulation domain
> > > in the radio card before each scan request:

Interesting.. I'm not completely sure what could have changed the
behavior based on beacon hint. I thought it was cfg80211, but if the
simple change for doing NO_IR | RADAR is not sufficient, it would seem
to imply that something else can do this. Some more debugging to do, I
guess.

> The distinction between NO_IR and CHAN_RADAR is not very clear to me.
> NO_IR appears only in the world regulatory domain so it's not relevant here.

NO_IR is a combination of not allowing AP, IBSS, or active scanning
without having somehow been enabled by another device. RADAR has that
same impact and in addition, requirement for doing radar detection and
DFS by a master device.

> I'd say
>  "the CHAN_RADAR flag should always make the firmware never do IR when
> probing"
> ...maybe, except if the channel is the operating channel. (this should
> exclude
> unassociated stations)

For most cases, I'd agree that active scanning should not be used on DFS
channels. That said, unicast Probe Request frame to the current AP while
associated could be a reasonable exception. In addition, WPS with PBC
depends on Probe Request frames to allow PBC session overlap detection,
so there might be sufficient justification to allow Probe Request frame
to be sent out for a very short duration (couple of seconds) after
seeing a Beacon frame on the channel for such special cases.
Jean-Pierre TOSONI Dec. 26, 2016, 11:15 a.m. UTC | #4
> -----Message d'origine-----
> De : Jouni Malinen [mailto:j@w1.fi]
> Envoyé : jeudi 15 décembre 2016 23:58
> À : Jean-Pierre Tosoni
> Cc : 'Ben Greear'; linux-wireless@vger.kernel.org;
> ath10k@lists.infradead.org
> Objet : Re: ath10k firmware sends probes on DFS channels without radar
> detection
> 
> On Thu, Dec 15, 2016 at 06:53:47PM +0100, Jean-Pierre Tosoni wrote:
> > > > Thanks for the suggestion, I already tried something like this in
> > > > wmi.c, with the same result:
> > > >
> > > > - Before patching the firmware scans DFS channels actively (with
> > > probes).
> > > >
> > > > - After patching, the firmware scans DFS channels passively
> > > > *until* any beacon is received on the DFS channel. When *any*
> > > > beacon is seen, the firmware decides to scan actively on its own,
> > > > without any new IR/RADAR info from the driver.
> > > >
> > > > So, your patch is required but not sufficient.
> > > >
> > > > Somehow I was able to overcome this by reloading the regulation
> > > > domain in the radio card before each scan request:
> 
> Interesting.. I'm not completely sure what could have changed the behavior
> based on beacon hint. I thought it was cfg80211, but if the simple change
> for doing NO_IR | RADAR is not sufficient, it would seem to imply that
> something else can do this. Some more debugging to do, I guess.

After some debugging I think the card firmware does this, probably due to
the lack of precise definition of NO_IR, see below.

> 
> > The distinction between NO_IR and CHAN_RADAR is not very clear to me.
> > NO_IR appears only in the world regulatory domain so it's not relevant
> here.
> 
> NO_IR is a combination of not allowing AP, IBSS, or active scanning
> without having somehow been enabled by another device. RADAR has that same
> impact and in addition, requirement for doing radar detection and DFS by a
> master device.

Ah, thanks. But then, NO_IR does not define the way for the "other device"
to enable the local device? So, depending on the interpretation, it can
render the local device unusable. OTOH RADAR defines a way which depends on
the local regulations.

> 
> > I'd say
> >  "the CHAN_RADAR flag should always make the firmware never do IR when
> > probing"
> > ...maybe, except if the channel is the operating channel. (this should
> > exclude unassociated stations)
> 
> For most cases, I'd agree that active scanning should not be used on DFS
> channels. That said, unicast Probe Request frame to the current AP while
> associated could be a reasonable exception. In addition, WPS with PBC
> depends on Probe Request frames to allow PBC session overlap detection, so
> there might be sufficient justification to allow Probe Request frame to be
> sent out for a very short duration (couple of seconds) after seeing a
> Beacon frame on the channel for such special cases.

I agree that unicast probes to the current AP should go through. It goes
with my condition "operating channel".

For WPS, I do not know it well, but I guess probes are acceptable if
1) they are not sent repeatedly over a long period of time during
unassociated state,
2) the AP uses CAC.
And here, both seem to be true.

> 
> --
> Jouni Malinen                                            PGP id EFC895FA

Regards, Jean-Pierre
diff mbox

Patch

--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2842,7 +2842,9 @@  static int ath10k_update_channel_list(st
 			ch->chan_radar =
 				!!(channel->flags & IEEE80211_CHAN_RADAR);
 
-			passive = channel->flags & IEEE80211_CHAN_NO_IR;
+			passive = channel->flags & (IEEE80211_CHAN_NO_IR |
+						    IEEE80211_CHAN_RADAR);
+
 			ch->passive = passive;
 
 			ch->freq = channel->center_freq;
@@ -3548,6 +3550,9 @@  static int ath10k_start_scan(struct ath1
 
 	lockdep_assert_held(&ar->conf_mutex);
 
+	if (ar->state == ATH10K_STATE_ON)
+		ath10k_regd_update(ar);
+
 	ret = ath10k_wmi_start_scan(ar, arg);
 	if (ret)
 		return ret;