[PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ath10k_mac_vif_chan() always returns an error for the given vif
during system-wide resume which reliably triggers two WARN_ON()s
in ath10k_bss_info_changed() and they are not particularly
useful in that code path, so drop them.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/net/wireless/ath/ath10k/mac.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/net/wireless/ath/ath10k/mac.c
===================================================================
--- linux-pm.orig/drivers/net/wireless/ath/ath10k/mac.c
+++ linux-pm/drivers/net/wireless/ath/ath10k/mac.c
@@ -5705,7 +5705,7 @@ static void ath10k_bss_info_changed(stru
 	}
 
 	if (changed & BSS_CHANGED_MCAST_RATE &&
-	    !WARN_ON(ath10k_mac_vif_chan(arvif->vif, &def))) {
+	    !ath10k_mac_vif_chan(arvif->vif, &def)) {
 		band = def.chan->band;
 		rateidx = vif->bss_conf.mcast_rate[band] - 1;
 
@@ -5743,7 +5743,7 @@ static void ath10k_bss_info_changed(stru
 	}
 
 	if (changed & BSS_CHANGED_BASIC_RATES) {
-		if (WARN_ON(ath10k_mac_vif_chan(vif, &def))) {
+		if (ath10k_mac_vif_chan(vif, &def)) {
 			mutex_unlock(&ar->conf_mutex);
 			return;
 		}

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Brian Norris]

+ Sriram, Pradeep, Claire

On Sun, Mar 03, 2019 at 06:24:33PM +0100, Rafael J. Wysocki wrote:

Ooh, exactly 1 month ago!

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> ath10k_mac_vif_chan() always returns an error for the given vif
> during system-wide resume which reliably triggers two WARN_ON()s
> in ath10k_bss_info_changed() and they are not particularly
> useful in that code path, so drop them.
> 

Particularly, when WOWLAN isn't enabled, we get called during resume via
ieee80211_reconfig(), where we're not associated and don't have any
channel contexts. AFAICT, we shouldn't need to communicate anything in
particular to the firmware here, and so failing the 'if' is definitely
not worth WARN-ing about.

I'd love to see this get applied with:

Fixes: cd93b83ad927 ("ath10k: support for multicast rate control")
Fixes: f279294e9ee2 ("ath10k: add support for configuring management packet rate")

and sent to stable. This has been bugging people since 4.19. Spurious
WARN_ON()s can trigger reports to various crash trackers, and on some
systems appear as user-visible warnings ("System problem detected").

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

> ---
>  drivers/net/wireless/ath/ath10k/mac.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/net/wireless/ath/ath10k/mac.c
> ===================================================================
> --- linux-pm.orig/drivers/net/wireless/ath/ath10k/mac.c
> +++ linux-pm/drivers/net/wireless/ath/ath10k/mac.c
> @@ -5705,7 +5705,7 @@ static void ath10k_bss_info_changed(stru
>  	}
>  
>  	if (changed & BSS_CHANGED_MCAST_RATE &&
> -	    !WARN_ON(ath10k_mac_vif_chan(arvif->vif, &def))) {
> +	    !ath10k_mac_vif_chan(arvif->vif, &def)) {
>  		band = def.chan->band;
>  		rateidx = vif->bss_conf.mcast_rate[band] - 1;
>  
> @@ -5743,7 +5743,7 @@ static void ath10k_bss_info_changed(stru
>  	}
>  
>  	if (changed & BSS_CHANGED_BASIC_RATES) {
> -		if (WARN_ON(ath10k_mac_vif_chan(vif, &def))) {
> +		if (ath10k_mac_vif_chan(vif, &def)) {
>  			mutex_unlock(&ar->conf_mutex);
>  			return;
>  		}
> 

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Rafael J. Wysocki]

On Wednesday, April 3, 2019 9:57:20 PM CEST Brian Norris wrote:
> + Sriram, Pradeep, Claire
> 
> On Sun, Mar 03, 2019 at 06:24:33PM +0100, Rafael J. Wysocki wrote:
> 
> Ooh, exactly 1 month ago!
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > ath10k_mac_vif_chan() always returns an error for the given vif
> > during system-wide resume which reliably triggers two WARN_ON()s
> > in ath10k_bss_info_changed() and they are not particularly
> > useful in that code path, so drop them.
> > 
> 
> Particularly, when WOWLAN isn't enabled, we get called during resume via
> ieee80211_reconfig(), where we're not associated and don't have any
> channel contexts. AFAICT, we shouldn't need to communicate anything in
> particular to the firmware here, and so failing the 'if' is definitely
> not worth WARN-ing about.
> 
> I'd love to see this get applied with:
> 
> Fixes: cd93b83ad927 ("ath10k: support for multicast rate control")
> Fixes: f279294e9ee2 ("ath10k: add support for configuring management packet rate")
> 
> and sent to stable. This has been bugging people since 4.19. Spurious
> WARN_ON()s can trigger reports to various crash trackers, and on some
> systems appear as user-visible warnings ("System problem detected").
> 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>

Thanks Brian, appreciated!

I have been wondering whether or not I need to resend this patch or something ...

> > ---
> >  drivers/net/wireless/ath/ath10k/mac.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Index: linux-pm/drivers/net/wireless/ath/ath10k/mac.c
> > ===================================================================
> > --- linux-pm.orig/drivers/net/wireless/ath/ath10k/mac.c
> > +++ linux-pm/drivers/net/wireless/ath/ath10k/mac.c
> > @@ -5705,7 +5705,7 @@ static void ath10k_bss_info_changed(stru
> >  	}
> >  
> >  	if (changed & BSS_CHANGED_MCAST_RATE &&
> > -	    !WARN_ON(ath10k_mac_vif_chan(arvif->vif, &def))) {
> > +	    !ath10k_mac_vif_chan(arvif->vif, &def)) {
> >  		band = def.chan->band;
> >  		rateidx = vif->bss_conf.mcast_rate[band] - 1;
> >  
> > @@ -5743,7 +5743,7 @@ static void ath10k_bss_info_changed(stru
> >  	}
> >  
> >  	if (changed & BSS_CHANGED_BASIC_RATES) {
> > -		if (WARN_ON(ath10k_mac_vif_chan(vif, &def))) {
> > +		if (ath10k_mac_vif_chan(vif, &def)) {
> >  			mutex_unlock(&ar->conf_mutex);
> >  			return;
> >  		}
> > 
> 

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Brian Norris]

On Wed, Apr 3, 2019 at 2:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> I have been wondering whether or not I need to resend this patch or something ...

I think the linux-wireless Patchwork instance is the source of truth
-- Kalle and Johannes use it for tracking status, IIUC. So this says
your patch is still "New":

https://patchwork.kernel.org/patch/10837139/

Brian

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> On Wed, Apr 3, 2019 at 2:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> I have been wondering whether or not I need to resend this patch or something ...
>
> I think the linux-wireless Patchwork instance is the source of truth
> -- Kalle and Johannes use it for tracking status, IIUC. So this says
> your patch is still "New":
>
> https://patchwork.kernel.org/patch/10837139/

Yeah, that's right:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork

So no need to resend, it's in my queue.

I was travelling and then busy with something else so I haven't had a
chance to process patches, I'll try to catch up now but I'm sure it will
take a while as there are so many patches. Sorry for the delay.

-- 
Kalle Valo

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Rafael J. Wysocki]

On Thu, Apr 4, 2019 at 6:08 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Brian Norris <briannorris@chromium.org> writes:
>
> > On Wed, Apr 3, 2019 at 2:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> I have been wondering whether or not I need to resend this patch or something ...
> >
> > I think the linux-wireless Patchwork instance is the source of truth
> > -- Kalle and Johannes use it for tracking status, IIUC. So this says
> > your patch is still "New":
> >
> > https://patchwork.kernel.org/patch/10837139/
>
> Yeah, that's right:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork
>
> So no need to resend, it's in my queue.
>
> I was travelling and then busy with something else so I haven't had a
> chance to process patches, I'll try to catch up now but I'm sure it will
> take a while as there are so many patches. Sorry for the delay.

No worries. :-)

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> + Sriram, Pradeep, Claire
>
> On Sun, Mar 03, 2019 at 06:24:33PM +0100, Rafael J. Wysocki wrote:
>
> Ooh, exactly 1 month ago!
>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> 
>> ath10k_mac_vif_chan() always returns an error for the given vif
>> during system-wide resume which reliably triggers two WARN_ON()s
>> in ath10k_bss_info_changed() and they are not particularly
>> useful in that code path, so drop them.
>> 
>
> Particularly, when WOWLAN isn't enabled, we get called during resume via
> ieee80211_reconfig(), where we're not associated and don't have any
> channel contexts. AFAICT, we shouldn't need to communicate anything in
> particular to the firmware here, and so failing the 'if' is definitely
> not worth WARN-ing about.
>
> I'd love to see this get applied with:
>
> Fixes: cd93b83ad927 ("ath10k: support for multicast rate control")
> Fixes: f279294e9ee2 ("ath10k: add support for configuring management packet rate")
>
> and sent to stable. This has been bugging people since 4.19. Spurious
> WARN_ON()s can trigger reports to various crash trackers, and on some
> systems appear as user-visible warnings ("System problem detected").
>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>

I added these now to the commit log, thanks Brian.

Rafael, could you please provide the hardware and firmware versions you
tested this on? We have so many different firmware branches to support
that I prefer to have that documented in the commit log. Providing
ath10k startup messages in dmesg are enough, I can then add it to the
commit log.

-- 
Kalle Valo

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Rafael J. Wysocki]

On Fri, Apr 26, 2019 at 9:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Brian Norris <briannorris@chromium.org> writes:
>
> > + Sriram, Pradeep, Claire
> >
> > On Sun, Mar 03, 2019 at 06:24:33PM +0100, Rafael J. Wysocki wrote:
> >
> > Ooh, exactly 1 month ago!
> >
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> ath10k_mac_vif_chan() always returns an error for the given vif
> >> during system-wide resume which reliably triggers two WARN_ON()s
> >> in ath10k_bss_info_changed() and they are not particularly
> >> useful in that code path, so drop them.
> >>
> >
> > Particularly, when WOWLAN isn't enabled, we get called during resume via
> > ieee80211_reconfig(), where we're not associated and don't have any
> > channel contexts. AFAICT, we shouldn't need to communicate anything in
> > particular to the firmware here, and so failing the 'if' is definitely
> > not worth WARN-ing about.
> >
> > I'd love to see this get applied with:
> >
> > Fixes: cd93b83ad927 ("ath10k: support for multicast rate control")
> > Fixes: f279294e9ee2 ("ath10k: add support for configuring management packet rate")
> >
> > and sent to stable. This has been bugging people since 4.19. Spurious
> > WARN_ON()s can trigger reports to various crash trackers, and on some
> > systems appear as user-visible warnings ("System problem detected").
> >
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > Tested-by: Brian Norris <briannorris@chromium.org>
>
> I added these now to the commit log, thanks Brian.
>
> Rafael, could you please provide the hardware and firmware versions you
> tested this on? We have so many different firmware branches to support
> that I prefer to have that documented in the commit log. Providing
> ath10k startup messages in dmesg are enough,

There you go:

[    4.695349] ath10k_pci 0000:3a:00.0: enabling device (0000 -> 0002)
[    4.698165] ath10k_pci 0000:3a:00.0: pci irq msi oper_irq_mode 2
irq_mode 0 reset_mode 0
[    4.912240] ath10k_pci 0000:3a:00.0: qca6174 hw3.2 target
0x05030000 chip_id 0x00340aff sub 1a56:1535
[    4.912255] ath10k_pci 0000:3a:00.0: kconfig debug 0 debugfs 0
tracing 0 dfs 0 testmode 0
[    4.912716] ath10k_pci 0000:3a:00.0: firmware ver
WLAN.RM.2.0-00180-QCARMSWPZ-1 api 4 features
wowlan,ignore-otp,no-4addr-pad crc32 75dee6c5
[    4.982563] ath10k_pci 0000:3a:00.0: board_file api 2 bmi_id N/A
crc32 19644295

> I can then add it to the commit log.

Still, I'm quite sure that the WARN_ON()s trigger during system resume
regardless of the hw/fw combination.

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Claire Chang]

Tested-by: Claire Chang <tientzu@chromium.org>

> Still, I'm quite sure that the WARN_ON()s trigger during system resume
> regardless of the hw/fw combination.

Also see this on sido:

[    4.925278] ath10k_sdio mmc1:0001:1: qca6174 hw3.2 sdio target
0x05030000 chip_id 0x00000000 sub 0000:0000
[    4.935721] ath10k_sdio mmc1:0001:1: kconfig debug 1 debugfs 1
tracing 1 dfs 0 testmode 1
[    4.948750] ath10k_sdio mmc1:0001:1: firmware ver
WLAN.RMH.4.4.1-00007-QCARMSWP-1 api 6 features wowlan,ignore-otp crc32
b98adaf8
[    5.132728] ath10k_sdio mmc1:0001:1: board_file api 2 bmi_id 0:4
crc32 6364cfcc

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Kalle Valo]

Claire Chang <tientzu@chromium.org> writes:

> Tested-by: Claire Chang <tientzu@chromium.org>
>
>> Still, I'm quite sure that the WARN_ON()s trigger during system resume
>> regardless of the hw/fw combination.
>
> Also see this on sido:
>
> [    4.925278] ath10k_sdio mmc1:0001:1: qca6174 hw3.2 sdio target
> 0x05030000 chip_id 0x00000000 sub 0000:0000
> [    4.935721] ath10k_sdio mmc1:0001:1: kconfig debug 1 debugfs 1
> tracing 1 dfs 0 testmode 1
> [    4.948750] ath10k_sdio mmc1:0001:1: firmware ver
> WLAN.RMH.4.4.1-00007-QCARMSWP-1 api 6 features wowlan,ignore-otp crc32
> b98adaf8
> [    5.132728] ath10k_sdio mmc1:0001:1: board_file api 2 bmi_id 0:4
> crc32 6364cfcc

Great, thanks. I added your Tested-by as well.

-- 
Kalle Valo

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Kalle Valo]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Fri, Apr 26, 2019 at 9:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Brian Norris <briannorris@chromium.org> writes:
>>
>> > + Sriram, Pradeep, Claire
>> >
>> > On Sun, Mar 03, 2019 at 06:24:33PM +0100, Rafael J. Wysocki wrote:
>> >
>> > Ooh, exactly 1 month ago!
>> >
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >>
>> >> ath10k_mac_vif_chan() always returns an error for the given vif
>> >> during system-wide resume which reliably triggers two WARN_ON()s
>> >> in ath10k_bss_info_changed() and they are not particularly
>> >> useful in that code path, so drop them.
>> >>
>> >
>> > Particularly, when WOWLAN isn't enabled, we get called during resume via
>> > ieee80211_reconfig(), where we're not associated and don't have any
>> > channel contexts. AFAICT, we shouldn't need to communicate anything in
>> > particular to the firmware here, and so failing the 'if' is definitely
>> > not worth WARN-ing about.
>> >
>> > I'd love to see this get applied with:
>> >
>> > Fixes: cd93b83ad927 ("ath10k: support for multicast rate control")
>> > Fixes: f279294e9ee2 ("ath10k: add support for configuring management packet rate")
>> >
>> > and sent to stable. This has been bugging people since 4.19. Spurious
>> > WARN_ON()s can trigger reports to various crash trackers, and on some
>> > systems appear as user-visible warnings ("System problem detected").
>> >
>> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > Reviewed-by: Brian Norris <briannorris@chromium.org>
>> > Tested-by: Brian Norris <briannorris@chromium.org>
>>
>> I added these now to the commit log, thanks Brian.
>>
>> Rafael, could you please provide the hardware and firmware versions you
>> tested this on? We have so many different firmware branches to support
>> that I prefer to have that documented in the commit log. Providing
>> ath10k startup messages in dmesg are enough,
>
> There you go:
>
> [    4.695349] ath10k_pci 0000:3a:00.0: enabling device (0000 -> 0002)
> [    4.698165] ath10k_pci 0000:3a:00.0: pci irq msi oper_irq_mode 2
> irq_mode 0 reset_mode 0
> [    4.912240] ath10k_pci 0000:3a:00.0: qca6174 hw3.2 target
> 0x05030000 chip_id 0x00340aff sub 1a56:1535
> [    4.912255] ath10k_pci 0000:3a:00.0: kconfig debug 0 debugfs 0
> tracing 0 dfs 0 testmode 0
> [    4.912716] ath10k_pci 0000:3a:00.0: firmware ver
> WLAN.RM.2.0-00180-QCARMSWPZ-1 api 4 features
> wowlan,ignore-otp,no-4addr-pad crc32 75dee6c5
> [    4.982563] ath10k_pci 0000:3a:00.0: board_file api 2 bmi_id N/A
> crc32 19644295

Thanks, I added the info to the commit log.

>> I can then add it to the commit log.
>
> Still, I'm quite sure that the WARN_ON()s trigger during system resume
> regardless of the hw/fw combination.

Sure, I'm not claiming anything else. We just have so many different
hw/fw combos that I want to have the environment documented in the
commit log in case we need to investigate history in the future.

-- 
Kalle Valo

Re: [PATCH] ath10k: Drop WARN_ON()s that always trigger during system resume

[Kalle Valo]

"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> ath10k_mac_vif_chan() always returns an error for the given vif
> during system-wide resume which reliably triggers two WARN_ON()s
> in ath10k_bss_info_changed() and they are not particularly
> useful in that code path, so drop them.
> 
> Tested: QCA6174 hw3.2 PCI with WLAN.RM.2.0-00180-QCARMSWPZ-1
> Tested: QCA6174 hw3.2 SDIO with WLAN.RMH.4.4.1-00007-QCARMSWP-1
> 
> Fixes: cd93b83ad927 ("ath10k: support for multicast rate control")
> Fixes: f279294e9ee2 ("ath10k: add support for configuring management packet rate")
> Cc: stable@vger.kernel.org
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Claire Chang <tientzu@chromium.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-current branch of ath.git, thanks.

9e80ad37f678 ath10k: Drop WARN_ON()s that always trigger during system resume

-- 
https://patchwork.kernel.org/patch/10837139/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches