Re: [PATCH] wifi: mwifiex: Use "scan_plans->iterations" for bgscan repeat count

[Jeff Chen <jeff.chen_1@nxp.com>]

On Mon, Apr 21, 2025 at 05:46:45 PM -0700, Brian Norris wrote:
> On Wed, Apr 16, 2025 at 11:54:25PM +0800, Jeff Chen wrote:
> > Updated the "mwifiex_cfg80211_sched_scan_start" function to assign
> > "bgscan_cfg->repeat_count" based on "scan_plans->iterations"
> > provided in the sched_scan settings instead of the default
> > "MWIFIEX_BGSCAN_REPEAT_COUNT". This change ensures that the repeat
> > count aligns with the iterations specified in the schedule scan
> > plans.
> > 
> > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > index a099fdaafa45..be28c841c299 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > @@ -2833,7 +2833,7 @@ mwifiex_cfg80211_sched_scan_start(struct wiphy *wiphy,
> >  				request->scan_plans->interval :
> >  				MWIFIEX_BGSCAN_INTERVAL;
> >  
> > -	bgscan_cfg->repeat_count = MWIFIEX_BGSCAN_REPEAT_COUNT;
> 
> Drop the MWIFIEX_BGSCAN_REPEAT_COUNT definition from main.h, now that
> it's unused.

Thanks, I'll remove the MWIFIEX_BGSCAN_REPEAT_COUNT definition, as it's
no longer needed.

> 
> > +	bgscan_cfg->repeat_count = request->scan_plans->iterations;
> 
> Are you sure you want to take the provided value as-is? For one, the
> request field is 32 bits wide, and your FW interface is 16 bits, so we
> definitely to make some size checks at a minimum.

You're right about the size mismatch. I'll add proper size checks to
ensure that the value fits within the firmware interface constraints.
 
> It seems like we should be setting wiphy->max_sched_scan_plan_iterations
> somewhere...

I'll also set "wiphy->max_sched_scan_plan_iterations" accordingly.

 
> Additionaly, what about the described behavior for 0 in cfg80211.h?
> 
>  * @iterations: number of scan iterations in this scan plan. Zero means
>  *      infinite loop.
>  *      The last scan plan will always have this parameter set to zero,
>  *      all other scan plans will have a finite number of iterations.
> 
> Is that how FW treats a value of 0? Or is there some other sentinel
> value?

Yes, in the firmware, "0" also means infinite loop.

 
> And, why did we have "6" here previously? Is that an important default?
> Or was it just a guess, and it's really OK to just have 0 (infinite)
> default? This could be a user-noticeable change, but maybe that's OK.
> You should at least acknowledge how and why this will change things in
> real terms.

As for the previous default value, I couldn't find any specific discussion
on this in the relevant mailing lists,
https://lore.kernel.org/linux-wireless/1452677217-21439-2-git-send-email-akarwar@marvell.com/
Even in our downstream driver implementation, we don't set a default value;
instead, we rely on "scan_plans->iterations".
 
> All in all, it feels like you haven't given me much reasoning to say,
> "yes, this is correct and a good idea."
> 
> Brian
> 

Thanks for your valuable feedback.

Jeff