* [PATCH v2 1/7] wl12xx: implement change_interface
@ 2011-12-19 10:00 Eliad Peller
2011-12-19 10:59 ` Luciano Coelho
0 siblings, 1 reply; 6+ messages in thread
From: Eliad Peller @ 2011-12-19 10:00 UTC (permalink / raw)
To: Luciano Coelho; +Cc: linux-wireless
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);
+}
+
static int wl1271_join(struct wl1271 *wl, struct wl12xx_vif *wlvif,
bool set_assoc)
{
@@ -4629,6 +4640,7 @@ static const struct ieee80211_ops wl1271_ops = {
.stop = wl1271_op_stop,
.add_interface = wl1271_op_add_interface,
.remove_interface = wl1271_op_remove_interface,
+ .change_interface = wl12xx_op_change_interface,
#ifdef CONFIG_PM
.suspend = wl1271_op_suspend,
.resume = wl1271_op_resume,
--
1.7.6.401.g6a319
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/7] wl12xx: implement change_interface
2011-12-19 10:00 [PATCH v2 1/7] wl12xx: implement change_interface Eliad Peller
@ 2011-12-19 10:59 ` Luciano Coelho
2011-12-19 11:17 ` Eliad Peller
0 siblings, 1 reply; 6+ messages in thread
From: Luciano Coelho @ 2011-12-19 10:59 UTC (permalink / raw)
To: Eliad Peller; +Cc: linux-wireless
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).
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.
--
Cheers,
Luca.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/7] wl12xx: implement change_interface
2011-12-19 10:59 ` Luciano Coelho
@ 2011-12-19 11:17 ` Eliad Peller
2011-12-19 11:37 ` Luciano Coelho
2011-12-19 16:06 ` Johannes Berg
0 siblings, 2 replies; 6+ messages in thread
From: Eliad Peller @ 2011-12-19 11:17 UTC (permalink / raw)
To: Luciano Coelho; +Cc: linux-wireless
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.
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 :)
> 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.
Eliad.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/7] wl12xx: implement change_interface
2011-12-19 11:17 ` Eliad Peller
@ 2011-12-19 11:37 ` Luciano Coelho
2011-12-19 16:06 ` Johannes Berg
1 sibling, 0 replies; 6+ messages in thread
From: Luciano Coelho @ 2011-12-19 11:37 UTC (permalink / raw)
To: Eliad Peller; +Cc: linux-wireless
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/7] wl12xx: implement change_interface
2011-12-19 11:17 ` Eliad Peller
2011-12-19 11:37 ` Luciano Coelho
@ 2011-12-19 16:06 ` Johannes Berg
2011-12-19 16:21 ` Luciano Coelho
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2011-12-19 16:06 UTC (permalink / raw)
To: Eliad Peller; +Cc: Luciano Coelho, linux-wireless
On Mon, 2011-12-19 at 13:17 +0200, Eliad Peller wrote:
> >> 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.
> 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 :)
The reason I did it this way is because it simplified the code in the
driver (for me at least) -- I could call functions that work on a vif
and look at vif->type/p2p before and after doing my own changes, whereas
if mac80211 was doing the modification I'd have to add extra arguments
everywhere.
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/7] wl12xx: implement change_interface
2011-12-19 16:06 ` Johannes Berg
@ 2011-12-19 16:21 ` Luciano Coelho
0 siblings, 0 replies; 6+ messages in thread
From: Luciano Coelho @ 2011-12-19 16:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: Eliad Peller, linux-wireless
On Mon, 2011-12-19 at 17:06 +0100, Johannes Berg wrote:
> On Mon, 2011-12-19 at 13:17 +0200, Eliad Peller wrote:
>
> > >> 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.
> > 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 :)
>
> The reason I did it this way is because it simplified the code in the
> driver (for me at least) -- I could call functions that work on a vif
> and look at vif->type/p2p before and after doing my own changes, whereas
> if mac80211 was doing the modification I'd have to add extra arguments
> everywhere.
Yeah, in our driver it's simpler to do it like this too. It's a bit
weird, but I don't think it's a problem. And now that one, two, three
drivers do it the same way, it has become the de-facto way of doing
it. ;)
Would deserve a comment somewhere, IMHO, though.
--
Cheers,
Luca.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-12-19 16:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-19 10:00 [PATCH v2 1/7] wl12xx: implement change_interface Eliad Peller
2011-12-19 10:59 ` Luciano Coelho
2011-12-19 11:17 ` Eliad Peller
2011-12-19 11:37 ` Luciano Coelho
2011-12-19 16:06 ` Johannes Berg
2011-12-19 16:21 ` Luciano Coelho
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.