Re: [PATCH 1/2] cfg80211: ignore netif running state when changing iftype

[Johannes Berg <johannes@sipsolutions.net>]

On Thu, 2015-05-21 at 09:44 +0200, Michal Kazior wrote:
> On 20 May 2015 at 15:19, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Wed, 2015-05-20 at 15:17 +0200, Johannes Berg wrote:
> >
> >> > -   if (ntype != otype && netif_running(dev)) {
> >> > +   if (ntype != otype) {
> >> >             dev->ieee80211_ptr->use_4addr = false;
> >> >             dev->ieee80211_ptr->mesh_id_up_len = 0;
> >> >             wdev_lock(dev->ieee80211_ptr);
> >>
> >> I don't think that makes much sense - the code within this block really
> >> only makes sense when the interface *is* running, like disconnecting
> >> etc. Doing that when it's *not* would be entirely unexpected to the
> >> drivers, no?
> 
> The netif_running() was originally introduced in f8cdddb8d61d which
> did in for entirely different purpose. Hence stripping netif_running()
> shouldn't be a problem for drivers, can it? The patch was also made
> kind of obsolete by b6a550156bc. Perhaps there are some other
> behavioral changes that I'm unaware of in stuff that gets called upon
> entering this condition down the stack..?

Perhaps it doesn't matter actually, since all the functions that are
called here will double-check things before calling the driver?

> Maybe it's just safer to move use_4addr/mesh_id_up_len clearing into a
> separate `oftype != ntype` condition. What do you think?

Or maybe we should just look at the functions we call in more detail :)

> > The real problem here might be the assignment to use_4addr *before*
> > we've actually disconnected or anything, perhaps that should be moved?
> >
> > Similarly, the mesh_id_up_len should probably be moved into the mesh
> > point switch case...
> 
> The problem isn't use_4addr clearing ordering per se. The problem is
> it wasn't cleared at all on interface changes. 

Ah.

> Do you want me to put the above into the commit log? Should I put a
> copy into the mac80211 patch as well?

I think just noting (more explicitly) that it was *missing* a clearing
in these cases would have been helpful.

johannes