* Re: [PATCH] Fix Bug 199967 - change WARN_ON(1) to IWL_ERR() [not found] <20180818033555.18110-1-nyet@nyet.org> @ 2018-08-18 6:10 ` Kalle Valo 2018-08-18 8:41 ` Luciano Coelho 1 sibling, 0 replies; 5+ messages in thread From: Kalle Valo @ 2018-08-18 6:10 UTC (permalink / raw) To: Nye Liu Cc: Johannes Berg, Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, David S. Miller, linux-wireless + linux-wireless Nye Liu <nyet@nyet.org> writes: > The TX_STATUS_FAIL_DEST_PS case fills logs with full backtraces, which > are pretty useless. Just do IWL_ERR() printk. > > Signed-off-by: Nye Liu <nyet@nyet.org> > --- > drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Please use prefix "iwlwifi: " and Cc linux-wireless. https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_title_is_wrong -- Kalle Valo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix Bug 199967 - change WARN_ON(1) to IWL_ERR() [not found] <20180818033555.18110-1-nyet@nyet.org> 2018-08-18 6:10 ` [PATCH] Fix Bug 199967 - change WARN_ON(1) to IWL_ERR() Kalle Valo @ 2018-08-18 8:41 ` Luciano Coelho 2018-08-18 16:11 ` Nye Liu 1 sibling, 1 reply; 5+ messages in thread From: Luciano Coelho @ 2018-08-18 8:41 UTC (permalink / raw) To: Nye Liu, Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless, Kalle Valo, David S. Miller, linux-wireless On Fri, 2018-08-17 at 20:35 -0700, Nye Liu wrote: > The TX_STATUS_FAIL_DEST_PS case fills logs with full backtraces, which > are pretty useless. Just do IWL_ERR() printk. > > Signed-off-by: Nye Liu <nyet@nyet.org> > --- > drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > index cf2591f2ac23..87044953e6b4 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > @@ -1407,8 +1407,10 @@ static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm, > /* the FW should have stopped the queue and not > * return this status > */ > - WARN_ON(1); > info->flags |= IEEE80211_TX_STAT_TX_FILTERED; > + IWL_ERR(mvm, "TX_STATUS_FAIL_DEST_PS: " > + "tid %d, status %x, flags %x\n", tid, status, > + info->flags); > break; > default: > break; I think this error is serious enough and we would like to catch it when it occurs so we can debug the actual cause. But I agree that we shouldn't be repeating it millions of times. What about just changing it to WARN_ON_ONCE() instead? -- Cheers, Luca. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix Bug 199967 - change WARN_ON(1) to IWL_ERR() 2018-08-18 8:41 ` Luciano Coelho @ 2018-08-18 16:11 ` Nye Liu 2018-08-18 16:52 ` Joe Perches 0 siblings, 1 reply; 5+ messages in thread From: Nye Liu @ 2018-08-18 16:11 UTC (permalink / raw) To: Luciano Coelho, Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless, Kalle Valo, David S. Miller, linux-wireless On 8/18/2018 1:41 AM, Luciano Coelho wrote: > On Fri, 2018-08-17 at 20:35 -0700, Nye Liu wrote: >> The TX_STATUS_FAIL_DEST_PS case fills logs with full backtraces, which >> are pretty useless. Just do IWL_ERR() printk. >> >> Signed-off-by: Nye Liu <nyet@nyet.org> >> --- >> drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c >> index cf2591f2ac23..87044953e6b4 100644 >> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c >> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c >> @@ -1407,8 +1407,10 @@ static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm, >> /* the FW should have stopped the queue and not >> * return this status >> */ >> - WARN_ON(1); >> info->flags |= IEEE80211_TX_STAT_TX_FILTERED; >> + IWL_ERR(mvm, "TX_STATUS_FAIL_DEST_PS: " >> + "tid %d, status %x, flags %x\n", tid, status, >> + info->flags); >> break; >> default: >> break; > I think this error is serious enough and we would like to catch it when > it occurs so we can debug the actual cause. > > But I agree that we shouldn't be repeating it millions of times. What > about just changing it to WARN_ON_ONCE() instead? > That would be fine, but IMO the WARN_ON() provides less information that the printk(). I'm not an IWL devel but there is limited information on the wifi state itself in the WARN() - just call stack and register information. Also, with WARN_ON_ONCE() the frequency of the error is masked. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix Bug 199967 - change WARN_ON(1) to IWL_ERR() 2018-08-18 16:11 ` Nye Liu @ 2018-08-18 16:52 ` Joe Perches 2018-08-18 18:41 ` Luciano Coelho 0 siblings, 1 reply; 5+ messages in thread From: Joe Perches @ 2018-08-18 16:52 UTC (permalink / raw) To: Nye Liu, Luciano Coelho, Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless, Kalle Valo, David S. Miller, linux-wireless On Sat, 2018-08-18 at 09:11 -0700, Nye Liu wrote: > On 8/18/2018 1:41 AM, Luciano Coelho wrote: > > > On Fri, 2018-08-17 at 20:35 -0700, Nye Liu wrote: > > > The TX_STATUS_FAIL_DEST_PS case fills logs with full backtraces, which > > > are pretty useless. Just do IWL_ERR() printk. > > > > > > Signed-off-by: Nye Liu <nyet@nyet.org> > > > --- > > > drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > > > index cf2591f2ac23..87044953e6b4 100644 > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > > > @@ -1407,8 +1407,10 @@ static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm, > > > /* the FW should have stopped the queue and not > > > * return this status > > > */ > > > - WARN_ON(1); > > > info->flags |= IEEE80211_TX_STAT_TX_FILTERED; > > > + IWL_ERR(mvm, "TX_STATUS_FAIL_DEST_PS: " > > > + "tid %d, status %x, flags %x\n", tid, status, > > > + info->flags); > > > break; > > > default: > > > break; > > > > I think this error is serious enough and we would like to catch it when > > it occurs so we can debug the actual cause. > > > > But I agree that we shouldn't be repeating it millions of times. What > > about just changing it to WARN_ON_ONCE() instead? > > > > That would be fine, but IMO the WARN_ON() provides less information that > the printk(). I'm not an IWL devel but there is limited information on > the wifi state itself in the WARN() - just call stack and register > information. Also, with WARN_ON_ONCE() the frequency of the error is masked. This could also use WARN_ON_RATELIMIT with some appropriate state. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix Bug 199967 - change WARN_ON(1) to IWL_ERR() 2018-08-18 16:52 ` Joe Perches @ 2018-08-18 18:41 ` Luciano Coelho 0 siblings, 0 replies; 5+ messages in thread From: Luciano Coelho @ 2018-08-18 18:41 UTC (permalink / raw) To: Joe Perches, Nye Liu, Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless, Kalle Valo, David S. Miller, linux-wireless On Sat, 2018-08-18 at 09:52 -0700, Joe Perches wrote: > On Sat, 2018-08-18 at 09:11 -0700, Nye Liu wrote: > > On 8/18/2018 1:41 AM, Luciano Coelho wrote: > > > > > On Fri, 2018-08-17 at 20:35 -0700, Nye Liu wrote: > > > > The TX_STATUS_FAIL_DEST_PS case fills logs with full > > > > backtraces, which > > > > are pretty useless. Just do IWL_ERR() printk. > > > > > > > > Signed-off-by: Nye Liu <nyet@nyet.org> > > > > --- > > > > drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > > > > b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > > > > index cf2591f2ac23..87044953e6b4 100644 > > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > > > > @@ -1407,8 +1407,10 @@ static void > > > > iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm, > > > > /* the FW should have stopped the queue > > > > and not > > > > * return this status > > > > */ > > > > - WARN_ON(1); > > > > info->flags |= > > > > IEEE80211_TX_STAT_TX_FILTERED; > > > > + IWL_ERR(mvm, "TX_STATUS_FAIL_DEST_PS: " > > > > + "tid %d, status %x, flags > > > > %x\n", tid, status, > > > > + info->flags); > > > > break; > > > > default: > > > > break; > > > > > > I think this error is serious enough and we would like to catch > > > it when > > > it occurs so we can debug the actual cause. > > > > > > But I agree that we shouldn't be repeating it millions of > > > times. What > > > about just changing it to WARN_ON_ONCE() instead? > > > > > > > That would be fine, but IMO the WARN_ON() provides less information > > that > > the printk(). I'm not an IWL devel but there is limited information > > on > > the wifi state itself in the WARN() - just call stack and register > > information. Also, with WARN_ON_ONCE() the frequency of the error > > is masked. > > This could also use WARN_ON_RATELIMIT with some > appropriate state. I think it's overkill. If it happens once, we will know and we will investigate. I don't see the added value of making it happen more than one, especially since it seems that the connection continues to work properly. I agree that passing more data in the warning could be useful, and that could be done with WARN_ONCE() instead of WARN_ON_ONCE() then. But the status is useless, we know it's TX_STATUS_FAIL_DEST_PS if we check where it happened. The flags are also not necessary, since it's data we are *setting* in this function. Maybe the tid, but I'm not sure it's really relevant either. -- Luca. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-18 21:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180818033555.18110-1-nyet@nyet.org>
2018-08-18 6:10 ` [PATCH] Fix Bug 199967 - change WARN_ON(1) to IWL_ERR() Kalle Valo
2018-08-18 8:41 ` Luciano Coelho
2018-08-18 16:11 ` Nye Liu
2018-08-18 16:52 ` Joe Perches
2018-08-18 18:41 ` Luciano Coelho
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).