diff mbox series

wifi: mwifiex: Use "scan_plans->iterations" for bgscan repeat count

Message ID 20250416155425.4070888-1-jeff.chen_1@nxp.com
State New
Headers show
Series wifi: mwifiex: Use "scan_plans->iterations" for bgscan repeat count | expand

Commit Message

Jeff Chen April 16, 2025, 3:54 p.m. UTC 
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(-)


base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8

Comments

Brian Norris April 22, 2025, 12:46 a.m. UTC | #1 
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
>
Jeff Chen April 29, 2025, 3:31 a.m. UTC | #2 
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
diff mbox series

Patch

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;
+	bgscan_cfg->repeat_count = request->scan_plans->iterations;
 	bgscan_cfg->report_condition = MWIFIEX_BGSCAN_SSID_MATCH |
 				MWIFIEX_BGSCAN_WAIT_ALL_CHAN_DONE;
 	bgscan_cfg->bss_type = MWIFIEX_BSS_MODE_INFRA;