From: Mohammed Shafi Shajakhan <[email protected]>
Previously we would check in nl80211 with an empty
cfg80211_wowlan structure to check for 'iw phy phyX wowlan enable'
with empty arguments (or) no triggers and disable WoWLAN.
It would be nice to inform the user that the wowlan enable is
provided with zero arugments in userspace itself rather than
disabling it in nl80211/cfg80211. A correspoding patch
was also proposed for 'iw' tool.
Signed-off-by: Mohammed Shafi Shajakhan <[email protected]>
---
net/wireless/nl80211.c | 31 +++++++++++++++++--------------
1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2a5cdb6..fb63f72 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6363,17 +6363,20 @@ static int nl80211_set_wowlan(struct sk_buff *skb, struct genl_info *info)
{
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct nlattr *tb[NUM_NL80211_WOWLAN_TRIG];
- struct cfg80211_wowlan no_triggers = {};
struct cfg80211_wowlan new_triggers = {};
+ struct cfg80211_wowlan *ntrig;
struct wiphy_wowlan_support *wowlan = &rdev->wiphy.wowlan;
int err, i;
bool prev_enabled = rdev->wowlan;
+ bool wow_disabled = false;
if (!rdev->wiphy.wowlan.flags && !rdev->wiphy.wowlan.n_patterns)
return -EOPNOTSUPP;
- if (!info->attrs[NL80211_ATTR_WOWLAN_TRIGGERS])
+ if (!info->attrs[NL80211_ATTR_WOWLAN_TRIGGERS]) {
+ wow_disabled = true;
goto no_triggers;
+ }
err = nla_parse(tb, MAX_NL80211_WOWLAN_TRIG,
nla_data(info->attrs[NL80211_ATTR_WOWLAN_TRIGGERS]),
@@ -6484,18 +6487,18 @@ static int nl80211_set_wowlan(struct sk_buff *skb, struct genl_info *info)
}
}
- if (memcmp(&new_triggers, &no_triggers, sizeof(new_triggers))) {
- struct cfg80211_wowlan *ntrig;
- ntrig = kmemdup(&new_triggers, sizeof(new_triggers),
- GFP_KERNEL);
- if (!ntrig) {
- err = -ENOMEM;
- goto error;
- }
- cfg80211_rdev_free_wowlan(rdev);
- rdev->wowlan = ntrig;
- } else {
- no_triggers:
+ ntrig = kmemdup(&new_triggers, sizeof(new_triggers),
+ GFP_KERNEL);
+ if (!ntrig) {
+ err = -ENOMEM;
+ goto error;
+ }
+ cfg80211_rdev_free_wowlan(rdev);
+ rdev->wowlan = ntrig;
+
+no_triggers:
+
+ if (wow_disabled) {
cfg80211_rdev_free_wowlan(rdev);
rdev->wowlan = NULL;
}
--
1.7.0.4
Hi,
> > Maybe something like this would make sense then?
> >
> > http://p.sipsolutions.net/2aac79bcfe3a9b8b.txt
>
> yeah this was the thing i am proposing
> http://www.spinics.net/lists/linux-wireless/msg94023.html
>
> your patch is better optimized as it avoids a bool variable to check for
> wow disable command, so this should be fine with the other iw
> patch to check for empty triggers ?
> http://www.spinics.net/lists/linux-wireless/msg94022.html
Well, my patch is different, it actually allows empty triggers to go
through as I had explained.
I'm just not sure which one makes more sense? I kinda feel that allowing
empty triggers might be a corner case but useful, while disallowing it
now would make it hard to ever support such a case?
johannes
Hi Johannes,
thanks for getting back to this :)
On Thursday 12 July 2012 07:54 PM, Johannes Berg wrote:
> On Wed, 2012-07-11 at 11:03 +0530, Mohammed Shafi Shajakhan wrote:
>
>>> I don't know if there's any advantage. It could be useful for example to
>>> get a connection up quicker after resume. I just didn't want to preclude
>>> that use case.
>>>
>>
>> sorry i just missed this thing. rdev->wowlan is made 'false' for empty
>
> I think you mean "NULL"?
oh yes.
>
> Maybe something like this would make sense then?
>
> http://p.sipsolutions.net/2aac79bcfe3a9b8b.txt
yeah this was the thing i am proposing
http://www.spinics.net/lists/linux-wireless/msg94023.html
your patch is better optimized as it avoids a bool variable to check for
wow disable command, so this should be fine with the other iw
patch to check for empty triggers ?
http://www.spinics.net/lists/linux-wireless/msg94022.html
thank you!
>
> johannes
>
--
thanks,
shafi
On Wed, 2012-07-11 at 11:03 +0530, Mohammed Shafi Shajakhan wrote:
> > I don't know if there's any advantage. It could be useful for example to
> > get a connection up quicker after resume. I just didn't want to preclude
> > that use case.
> >
>
> sorry i just missed this thing. rdev->wowlan is made 'false' for empty
I think you mean "NULL"?
Maybe something like this would make sense then?
http://p.sipsolutions.net/2aac79bcfe3a9b8b.txt
johannes
On Tue, 2012-07-10 at 16:46 +0530, Mohammed Shafi Shajakhan wrote:
> Hi Johannes,
>
> On Tuesday 10 July 2012 04:36 PM, Johannes Berg wrote:
> > On Tue, 2012-07-10 at 15:37 +0530, Mohammed Shafi Shajakhan wrote:
> >> From: Mohammed Shafi Shajakhan <[email protected]>
> >>
> >> Previously we would check in nl80211 with an empty
> >> cfg80211_wowlan structure to check for 'iw phy phyX wowlan enable'
> >> with empty arguments (or) no triggers and disable WoWLAN.
> >
> > No ... that's not how it works. If you enable WoWLAN without any
> > triggers, then that's what you get: the connection is kept alive but
> > there are no wakeup sources. If you don't specify the triggers at all
> > then it's disabled.
> >
>
> If we want to do disable WoWLAN we can use the command 'iw phy phyX
> wowlan disable' command. Its true that we would 'Keep Alive'
> the connection, but we would disable the wake up capability via
> 'set_wakeup' callback, something similar to wowlan disable command.
> Please let me know if i am missing something (or) the advantage having
> the connection Keep alive with wowlan enable without triggers.
I don't know if there's any advantage. It could be useful for example to
get a connection up quicker after resume. I just didn't want to preclude
that use case.
johannes
On Thursday 12 July 2012 08:47 PM, Johannes Berg wrote:
> Hi,
>
>>> Maybe something like this would make sense then?
>>>
>>> http://p.sipsolutions.net/2aac79bcfe3a9b8b.txt
>>
>> yeah this was the thing i am proposing
>> http://www.spinics.net/lists/linux-wireless/msg94023.html
>>
>> your patch is better optimized as it avoids a bool variable to check for
>> wow disable command, so this should be fine with the other iw
>> patch to check for empty triggers ?
>> http://www.spinics.net/lists/linux-wireless/msg94022.html
>
> Well, my patch is different, it actually allows empty triggers to go
> through as I had explained.
oops, sorry i misread it.
>
> I'm just not sure which one makes more sense? I kinda feel that allowing
> empty triggers might be a corner case but useful, while disallowing it
> now would make it hard to ever support such a case?
i am fine with your approach itself and don't want to break that use case.
>
> johannes
>
--
thanks,
shafi
Hi Johannes,
On Tuesday 10 July 2012 04:36 PM, Johannes Berg wrote:
> On Tue, 2012-07-10 at 15:37 +0530, Mohammed Shafi Shajakhan wrote:
>> From: Mohammed Shafi Shajakhan <[email protected]>
>>
>> Previously we would check in nl80211 with an empty
>> cfg80211_wowlan structure to check for 'iw phy phyX wowlan enable'
>> with empty arguments (or) no triggers and disable WoWLAN.
>
> No ... that's not how it works. If you enable WoWLAN without any
> triggers, then that's what you get: the connection is kept alive but
> there are no wakeup sources. If you don't specify the triggers at all
> then it's disabled.
>
If we want to do disable WoWLAN we can use the command 'iw phy phyX
wowlan disable' command. Its true that we would 'Keep Alive'
the connection, but we would disable the wake up capability via
'set_wakeup' callback, something similar to wowlan disable command.
Please let me know if i am missing something (or) the advantage having
the connection Keep alive with wowlan enable without triggers.
thank you!
--
thanks,
shafi
Hi Johannes,
>>
>> On Tuesday 10 July 2012 04:36 PM, Johannes Berg wrote:
>>> On Tue, 2012-07-10 at 15:37 +0530, Mohammed Shafi Shajakhan wrote:
>>>> From: Mohammed Shafi Shajakhan <[email protected]>
>>>>
>>>> Previously we would check in nl80211 with an empty
>>>> cfg80211_wowlan structure to check for 'iw phy phyX wowlan enable'
>>>> with empty arguments (or) no triggers and disable WoWLAN.
>>>
>>> No ... that's not how it works. If you enable WoWLAN without any
>>> triggers, then that's what you get: the connection is kept alive but
>>> there are no wakeup sources. If you don't specify the triggers at all
>>> then it's disabled.
>>>
>>
>> If we want to do disable WoWLAN we can use the command 'iw phy phyX
>> wowlan disable' command. Its true that we would 'Keep Alive'
>> the connection, but we would disable the wake up capability via
>> 'set_wakeup' callback, something similar to wowlan disable command.
>> Please let me know if i am missing something (or) the advantage having
>> the connection Keep alive with wowlan enable without triggers.
>
> I don't know if there's any advantage. It could be useful for example to
> get a connection up quicker after resume. I just didn't want to preclude
> that use case.
>
oh, ok fine.
--
thanks,
shafi
On Tue, 2012-07-10 at 15:37 +0530, Mohammed Shafi Shajakhan wrote:
> From: Mohammed Shafi Shajakhan <[email protected]>
>
> Previously we would check in nl80211 with an empty
> cfg80211_wowlan structure to check for 'iw phy phyX wowlan enable'
> with empty arguments (or) no triggers and disable WoWLAN.
No ... that's not how it works. If you enable WoWLAN without any
triggers, then that's what you get: the connection is kept alive but
there are no wakeup sources. If you don't specify the triggers at all
then it's disabled.
johannes
Hi Johannes,
>>
>> On Tuesday 10 July 2012 04:36 PM, Johannes Berg wrote:
>>> On Tue, 2012-07-10 at 15:37 +0530, Mohammed Shafi Shajakhan wrote:
>>>> From: Mohammed Shafi Shajakhan <[email protected]>
>>>>
>>>> Previously we would check in nl80211 with an empty
>>>> cfg80211_wowlan structure to check for 'iw phy phyX wowlan enable'
>>>> with empty arguments (or) no triggers and disable WoWLAN.
>>>
>>> No ... that's not how it works. If you enable WoWLAN without any
>>> triggers, then that's what you get: the connection is kept alive but
>>> there are no wakeup sources. If you don't specify the triggers at all
>>> then it's disabled.
>>>
>>
>> If we want to do disable WoWLAN we can use the command 'iw phy phyX
>> wowlan disable' command. Its true that we would 'Keep Alive'
>> the connection, but we would disable the wake up capability via
>> 'set_wakeup' callback, something similar to wowlan disable command.
>> Please let me know if i am missing something (or) the advantage having
>> the connection Keep alive with wowlan enable without triggers.
>
> I don't know if there's any advantage. It could be useful for example to
> get a connection up quicker after resume. I just didn't want to preclude
> that use case.
>
sorry i just missed this thing. rdev->wowlan is made 'false' for empty
wowlan enable triggers, then we would not call drv_suspend/resume
callback, this would prevent the device from retaining the connection.
I would just check this with ath9k.
--
thanks,
shafi