Re: [PATCH] mac80211: do not iterate active interfaces when in re-configure

[Kalle Valo <kvalo@codeaurora.org>]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2020-07-30 at 07:52 -0700, Ben Greear wrote:
>
>> > Consider
>> > 
>> > add interface wlan0
>> > add interface wlan1
>> > iterate active interfaces -> wlan0 wlan1
>> > add interface wlan2
>> > iterate active interfaces -> wlan0 wlan1 wlan2
>> > 
>> > If you apply this scenario to a restart, which ought to be functionally
>> > equivalent to the normal startup, just compressed in time, you're
>> > basically saying that today you get
>> > 
>> > add interface wlan0
>> > add interface wlan1
>> > iterate active interfaces -> wlan0 wlan1 wlan2 << problem here
>> > add interface wlan2
>> > iterate active interfaces -> wlan0 wlan1 wlan2
>> > 
>> > which yeah, totally seems wrong.
>> > 
>> > But fixing that to be
>> > 
>> > add interface wlan0
>> > add interface wlan1
>> > iterate active interfaces ->
>> > <nothing>
>> > add interface wlan2
>> > iterate active interfaces -> <nothing>
>> > (or
>> > maybe -> wlan0 wlan1 wlan2 if the reconfig already completed)
>> > 
>> > seems equally wrong?
>> 
>> So, looks like there is a flags option passed to the iterate logic,
>> and it is indeed called
>> directly from drivers. So, I could just add a new flag value, and |
>> it in when calling from ath10k.
>> 
>> I'm not sure it would really solve the second case, but at least in practice,
>> that one doesn't seem to be a problem with ath10k, and the first case *was*
>> a problem.
>> 
>> If that sounds OK to you, I'll work on the patch as described.
>
> Right, that'd be the option 2. I described earlier. I can live with that
> even if I'd prefer to fix it as per 1. to "make sense". But I guess
> there could even be "more legitimate" cases to not want to iterate while
> restarting, even if I'm not really sure where that'd make sense?
>
> I guess Kalle should comment on whether he'd accept that into the
> driver.
>
> Kalle, as you can see above mac80211 appears to be broken wrt. iterating
> "active" interfaces during a restart - the iteration considers all
> interfaces active that were active before the restart, not just the ones
> that were already re-added to the driver. Ben says this causes trouble
> in ath10k.
>
> IMHO the right fix for this would be to fix the iteration to only reach
> the ones that have been re-added, like I've said above. OTOH, Ben isn't
> really convinced that that's right, and has experience with a patch that
> makes mac80211 return *no* interfaces whatsoever in the iteration when
> done while in restart. Like I say there, it seems wrong to me.
>
> But depending on what ath10k actually _does_ with this list, perhaps
> it's not an issue. Perhaps it's just transient state that it derives
> from it, so if it does it again after the reconfig is completed, it
> would in fact get all the information it needed.
>
> I'm pretty sure this would break iwlwifi, so one option (less preferred)
> would be to add a flag to say "skip iteration in reconfig".

To me it sounds fine to have such flag in ath10k. I just want the ath10k
patch test tested with upstream ath10k and firmware because Ben's driver
and firmware might behave differently.

> actually does the driver know it's in reconfig? Perhaps it could even do
> that completely on its own?

We do have ar->state which tracks the reconfig process. For example, in
ath10k_reconfig_complete() we have this check:

	/* If device failed to restart it will be in a different state, e.g.
	 * ATH10K_STATE_WEDGED
	 */
	if (ar->state == ATH10K_STATE_RESTARTED) {
		ath10k_info(ar, "device successfully recovered\n");
		ar->state = ATH10K_STATE_ON;
		ieee80211_wake_queues(ar->hw);
	}

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches