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

[Ben Greear <greearb@candelatech.com>]

On 7/30/20 8:03 AM, Johannes Berg wrote:
> 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".
> 
> actually does the driver know it's in reconfig? Perhaps it could even do
> that completely on its own?
> 
> Anyway, the question is what you think about doing such a thing in the
> driver, if it fixes issues even if it's probably not really correct.
> 
> johannes

So, no response from Kalle on this for some time.  I thought I'd ping one
more time before I make an effort to create another out-of-tree patch.

Johannes, if you are OK with a new flag in mac80211, then I can patch ath10k-ct
driver regardless of whether other drivers use it.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com