Re: [PATCH v2 1/7] wl12xx: implement change_interface

[Luciano Coelho <coelho@ti.com>]

On Mon, 2011-12-19 at 13:17 +0200, Eliad Peller wrote: 
> On Mon, Dec 19, 2011 at 12:59 PM, Luciano Coelho <coelho@ti.com> wrote:
> > On Mon, 2011-12-19 at 12:00 +0200, Eliad Peller wrote:
> >> Implement the change_interface callback by simply removing the
> >> current vif and adding a new one after updating the vif type.
> >>
> >> Signed-off-by: Eliad Peller <eliad@wizery.com>
> >> ---
> >> v2: update vif->p2p as well
> >>
> >>  drivers/net/wireless/wl12xx/main.c |   12 ++++++++++++
> >>  1 files changed, 12 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> >> index c305841..82fc318 100644
> >> --- a/drivers/net/wireless/wl12xx/main.c
> >> +++ b/drivers/net/wireless/wl12xx/main.c
> >> @@ -2269,6 +2269,17 @@ out:
> >>       cancel_work_sync(&wl->recovery_work);
> >>  }
> >>
> >> +static int wl12xx_op_change_interface(struct ieee80211_hw *hw,
> >> +                                   struct ieee80211_vif *vif,
> >> +                                   enum nl80211_iftype new_type, bool p2p)
> >> +{
> >> +     wl1271_op_remove_interface(hw, vif);
> >> +
> >> +     vif->type = ieee80211_iftype_p2p(new_type, p2p);
> >> +     vif->p2p = p2p;
> >> +     return wl1271_op_add_interface(hw, vif);
> >> +}
> >> +
> >
> > As Arik already commented, this looks a bit weird.  Shouldn't the type
> > and p2p elements be changed in mac80211 itself? What about doing so if
> > the op_change_interface returns success? Currently, the opposite seems
> > to happen (ie. mac80211 changes type back to the original vif.type if
> > the op fails).
> >
> iwlegacy and ath9k are also setting vif->type and vif->p2p in a similar manner.
> add_interface(vif) assumes vif->type and vif->p2p are correct, so it
> makes sense to update the vif before adding it.

Right, it just came to my mind after I sent my reply that the reason was
the add_interface().  Still it looks weird.  There would be ways to fix
this properly, for example by splitting our add_interface() function and
passing the type and p2p boolean to the inner one.  Not worth the
effort, so let's not bother for now.


> i also think this behavior is a bit ugly, but this is the current
> mac80211 implementation, and i don't see a good enough reason to
> change it :)

Actually after looking more into the code, mac80211 is doing the right
thing.  Calling change interface with the original vif.  That's why the
new parameters are passed too.  Only if the call succeeds, mac80211
changes the parameters in the local data.

The ugly thing is in the hack of removing and re-adding the interface
with a different type.


> > Also, it seems that the p2p value will be overwritten in the
> > ieee80211_setup_sdata() which is called just after this op returns, so
> > there's no use to do it here.
> this is needed as add_interface(vif) might assume vif->p2p is correct.

Yep, got it.

-- 
Cheers,
Luca.