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

[Brian Norris <briannorris@chromium.org>]

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.

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

It seems like we should be setting wiphy->max_sched_scan_plan_iterations
somewhere...

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?

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.

All in all, it feels like you haven't given me much reasoning to say,
"yes, this is correct and a good idea."

Brian

>  	bgscan_cfg->report_condition = MWIFIEX_BGSCAN_SSID_MATCH |
>  				MWIFIEX_BGSCAN_WAIT_ALL_CHAN_DONE;
>  	bgscan_cfg->bss_type = MWIFIEX_BSS_MODE_INFRA;
> 
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> -- 
> 2.34.1
>