Re: [PATCH v2] wifi: mac80211: re-order unassigning channel in activate links

[Johannes Berg <johannes@sipsolutions.net>]

On Thu, 2024-12-05 at 20:43 +0530, Aditya Kumar Singh wrote:
> On 12/5/24 18:30, Johannes Berg wrote:
> > On Thu, 2024-12-05 at 12:43 +0100, Johannes Berg wrote:
> > > > 
> > > > Therefore, re-order the logic so that stations are handled first and then
> > > > channel is unassigned.
> > > > 
> > > 
> > > This causes memory leaks in my tests with iwlwifi.
> > > 
> > 
> > And also firmware crashes because the station is removed while it's
> > still being used.
> > 
> 
> So is this exposing some underlying issue with iwlwifi?

I don't think so?

> Or this change 
> will break drivers which does not group multiple hardware into single 
> wiphy?

Not necessarily, but it breaks iwlwifi because of the changed order of
operations, and what it does with the firmware.

I think the issue here is that we treat link active == link has channel
context in iwlwifi, and an active link in client mode requires a station
in firmware. Otherwise you cannot even deactivate a link, since that
requires sending an NDP to the AP, but if you don't have the AP STA you
can't do that ...

I guess the driver could be changed to treat station links as active
when they have the AP STA entry, but that seems ... difficult and
strange, it would make it different between AP and client modes?

Looking at your commit message more, I wonder if it really even makes
sense to *delete* the link when the channel context is unassigned,
rather than (similarly to iwlwifi) deactivating it and deleting it later
when it's actually removed (change_vif_links)? You do know which
hardware it is/was on, after all. And these two operations can *never*
be atomic. Removing the STAs first might be something that's appropriate
for AP mode, but I guess I'm more with iwlwifi here in that it doesn't
seem quite right for client mode?

> Also, how about non-ML scenario in iwlwifi? There, first station is 
> removed and then the interface goes down right?

It's not so much about the interface but the link, it seems.

johannes