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@xxxxxxxx> > > > > --- > > > > 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.
