Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <9574407e2be4c97b1ef2d9f73b5eecb08791ff56.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: iwlwifi:  Add debugging around scan failure
 warning.
From:   Johannes Berg <johannes@sipsolutions.net>
To:     greearb@candelatech.com, linux-wireless@vger.kernel.org
Date:   Wed, 15 Nov 2023 09:57:23 +0100
In-Reply-To: <20231114212309.2180281-1-greearb@candelatech.com>
References: <20231114212309.2180281-1-greearb@candelatech.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-11-14 at 13:23 -0800, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>=20
> Provide additional information that may help debugging why
> the WARN_ON is seen.
>=20
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/scan.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>=20
> 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_mv=
m *mvm,
>  			link_id =3D ffs(vif->active_links) - 1;
> =20
>  		link_info =3D 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 act=
ive-links: 0x%x\n",
> +				link_id, vif->active_links);
>=20

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).

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.

johannes