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@xxxxxxx> > > --- > > 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@xxxxxxxxxxx/ 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
