On Wed, 2023-11-15 at 08:01 -0800, Ben Greear wrote: > On 11/15/23 00:57, Johannes Berg wrote: > > On Tue, 2023-11-14 at 13:23 -0800, greearb@xxxxxxxxxxxxxxx wrote: > > > From: Ben Greear <greearb@xxxxxxxxxxxxxxx> > > > > > > Provide additional information that may help debugging why > > > the WARN_ON is seen. > > > > > > Signed-off-by: Ben Greear <greearb@xxxxxxxxxxxxxxx> > > > --- > > > drivers/net/wireless/intel/iwlwifi/mvm/scan.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/scan.c b/drivers/net/wireless/intel/iwlwifi/mvm/scan.c > > > index 75c5c58e14a5..ec24ece7c877 100644 > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/scan.c > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/scan.c > > > @@ -2356,7 +2356,10 @@ iwl_mvm_scan_umac_fill_general_p_v12(struct iwl_mvm *mvm, > > > link_id = ffs(vif->active_links) - 1; > > > > > > link_info = scan_vif->link[link_id]; > > > - if (!WARN_ON(!link_info)) > > > + if (WARN_ON(!link_info)) > > > + IWL_ERR(mvm, "scan failure: Cannot find link info for link-id: %d active-links: 0x%x\n", > > > + link_id, vif->active_links); > > > > > > > It would make sense to put the data *into* the warning, rather than > > separately? Though I'm not sure I see so much value in the long string > > (vs. just the data). > > I assume IWL_ERR will provide some extra context in systems with multiple radios, > so more useful than what I could easily put into WARN_* directly. That's true, though not sure that helps so much more? You could put it easily into the warning though :) Anyway, the point is that bug reports from the field often don't include the full log etc. so putting things _into_ the warning really is usually more useful than putting it after. > > > > But honestly I'm not sure this really even is a problem at all? Some > > confusion can happen during firmware restart here, and not sure we can > > really fix that. Though maybe with the new locking we can. > > I see the splat, not sure why. It would be nice to know if link-id is > corrupted somehow, or if active links is zero, etc. Yeah in this case we'd really need more of the dynamic information to really debug it, I guess. johannes
