| Message ID | 2884043.Jv1Mn93hE8@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ath10k: Drop WARN_ON()s that always trigger during system resume | expand |
+ 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; > } >
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; > > } > > >
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
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.
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. :-)
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.
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.
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
"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.
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.
"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
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; }