From: Raja Mani <[email protected]>
* TX operation (ctrl tx and data tx) has to be controlled based on
WOW suspend state. i.e, control packets are allowed to send from
the host until the suspend state goes ATH6KL_STATE_WOW and
the data packets are allowed until WOW suspend operation starts.
* Similary, wow resume is NOT allowed if WOW suspend is in progress.
Both of the above scenarios are taken care in this patch.
Signed-off-by: Raja Mani <[email protected]>
---
drivers/net/wireless/ath/ath6kl/cfg80211.c | 14 ++++++++++++++
drivers/net/wireless/ath/ath6kl/core.c | 2 ++
drivers/net/wireless/ath/ath6kl/core.h | 7 +++++++
drivers/net/wireless/ath/ath6kl/txrx.c | 11 ++++++++++-
4 files changed, 33 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index d1922d8..11d9670 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -1941,6 +1941,8 @@ static int ath6kl_wow_suspend(struct ath6kl *ar, struct cfg80211_wowlan *wow)
if (ret)
return ret;
+ ar->wow_state = ATH6KL_WOW_STATE_SUSPENDING;
+
/* Setup own IP addr for ARP agent. */
in_dev = __in_dev_get_rtnl(vif->ndev);
if (!in_dev)
@@ -2015,10 +2017,15 @@ static int ath6kl_wow_resume(struct ath6kl *ar)
struct ath6kl_vif *vif;
int ret;
+ if (ar->wow_state == ATH6KL_WOW_STATE_NONE)
+ return 0;
+
vif = ath6kl_vif_first(ar);
if (!vif)
return -EIO;
+ ar->wow_state = ATH6KL_WOW_STATE_NONE;
+
ret = ath6kl_wmi_set_host_sleep_mode_cmd(ar->wmi, vif->fw_vif_idx,
ATH6KL_HOST_MODE_AWAKE);
return ret;
@@ -2041,9 +2048,13 @@ int ath6kl_cfg80211_suspend(struct ath6kl *ar,
ret = ath6kl_wow_suspend(ar, wow);
if (ret) {
ath6kl_err("wow suspend failed: %d\n", ret);
+ ar->wow_state = ATH6KL_WOW_STATE_NONE;
return ret;
}
+
+ ar->wow_state = ATH6KL_WOW_STATE_SUSPENDED;
ar->state = ATH6KL_STATE_WOW;
+
break;
case ATH6KL_CFG_SUSPEND_DEEPSLEEP:
@@ -2188,6 +2199,9 @@ static int __ath6kl_cfg80211_resume(struct wiphy *wiphy)
*/
void ath6kl_check_wow_status(struct ath6kl *ar)
{
+ if (ar->wow_state == ATH6KL_WOW_STATE_SUSPENDING)
+ return;
+
if (ar->state == ATH6KL_STATE_WOW)
ath6kl_cfg80211_resume(ar);
}
diff --git a/drivers/net/wireless/ath/ath6kl/core.c b/drivers/net/wireless/ath/ath6kl/core.c
index 722ca59..b522135 100644
--- a/drivers/net/wireless/ath/ath6kl/core.c
+++ b/drivers/net/wireless/ath/ath6kl/core.c
@@ -154,6 +154,8 @@ int ath6kl_core_init(struct ath6kl *ar)
else
ar->suspend_mode = 0;
+ ar->wow_state = ATH6KL_WOW_STATE_NONE;
+
if (uart_debug)
ar->conf_flags |= ATH6KL_CONF_UART_DEBUG;
diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index c4d66e0..182ff01 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -527,11 +527,18 @@ enum ath6kl_state {
ATH6KL_STATE_SCHED_SCAN,
};
+enum ath6kl_wow_state {
+ ATH6KL_WOW_STATE_NONE,
+ ATH6KL_WOW_STATE_SUSPENDING,
+ ATH6KL_WOW_STATE_SUSPENDED,
+};
+
struct ath6kl {
struct device *dev;
struct wiphy *wiphy;
enum ath6kl_state state;
+ enum ath6kl_wow_state wow_state;
unsigned int testmode;
struct ath6kl_bmi bmi;
diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index a3dc694..53a2894 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -284,6 +284,10 @@ int ath6kl_control_tx(void *devt, struct sk_buff *skb,
int status = 0;
struct ath6kl_cookie *cookie = NULL;
+#ifdef CONFIG_PM
+ if (ar->wow_state == ATH6KL_WOW_STATE_SUSPENDED)
+ return -EACCES;
+#endif
spin_lock_bh(&ar->lock);
ath6kl_dbg(ATH6KL_DBG_WLAN_TX,
@@ -352,7 +356,12 @@ int ath6kl_data_tx(struct sk_buff *skb, struct net_device *dev)
ath6kl_dbg(ATH6KL_DBG_WLAN_TX,
"%s: skb=0x%p, data=0x%p, len=0x%x\n", __func__,
skb, skb->data, skb->len);
-
+#ifdef CONFIG_PM
+ if (ar->wow_state != ATH6KL_WOW_STATE_NONE) {
+ dev_kfree_skb(skb);
+ return 0;
+ }
+#endif
/* If target is not associated */
if (!test_bit(CONNECTED, &vif->flags)) {
dev_kfree_skb(skb);
--
1.7.1
On Tuesday 07 February 2012 07:03 PM, Kalle Valo wrote:
> On 02/07/2012 12:14 PM, Raja Mani wrote:
>> On Monday 06 February 2012 07:44 PM, Kalle Valo wrote:
>>> On 02/06/2012 03:56 PM, [email protected] wrote:
>>>>
>>>> +enum ath6kl_wow_state {
>>>> + ATH6KL_WOW_STATE_NONE,
>>>> + ATH6KL_WOW_STATE_SUSPENDING,
>>>> + ATH6KL_WOW_STATE_SUSPENDED,
>>>> +};
>>>> +
>>>> struct ath6kl {
>>>> struct device *dev;
>>>> struct wiphy *wiphy;
>>>>
>>>> enum ath6kl_state state;
>>>> + enum ath6kl_wow_state wow_state;
>>>> unsigned int testmode;
>>>
>>> To be honest, adding a new state variable scares me. I don't see how we
>>> are able to maintain two different state variables, the end result would
>>> be a total mess.
>>
>> ath6kl_wow_state is a just sub state of WOW. It roles over only in WOW
>> mode. However i understand your point.
>
> It's not just a substate as it's separately checked in txrx.c.
>
>>> I recommend to look at this problem by adding a new state to enum
>>> ath6kl_state. That would make it a lot easier to handle all the
>>> different states.
>>
>> The condition to stop ctrl and data pkt transfer are different.
>> Ctrl pkt should be stopped when wow_suspended (after sending
>> set_host_sleep_cmd_mode). Data pkt should be dropped before
>> the moment we configure set_ip_cmd().
>
> Don't think about dropping packets, instead try to make sure that the
> packet flow is _stopped_. Dropping packets should be the last resort,
> instead we need to go to the source of the packets. For data packets
> this means that we need to stop netdev queues. For control packets we
> need to make sure that cfg80211 ops in cfg80211.c don't issue any new
> wmi commands. And maybe we should also consider debugfs and wmi events
> as they can also issue new wmi commands.
>
>> This where we need a state WOW_SUSPENDING and WOW_SUSPENDED.
>> enum ath6kl_state has over all ath6kl suspend state (cut pwr, deep
>> sleep,wow). IMHO, mixing WOW sub states there is not good approach.
>
> How is this problem related only to WoW? Doesn't it apply to all suspend
> modes?
>
>> If you feel maintaining separate state is not good idea, i could think
>> of introducing two new flag WOW_SUSPENDING, WOW_SUSPENDED in ar->flag.
>
> After a quick thought having a SUSPENDING state makes, but I can't see
> the reason for name WOW_SUSPENDING. WOW_SUSPENDED is unclear for me,
> what's the difference to ATH6KL_STATE_WOW?
Thanks your review. I'll submit V2 by considering all your comments.
>
> Kalle
On 02/07/2012 12:14 PM, Raja Mani wrote:
> On Monday 06 February 2012 07:44 PM, Kalle Valo wrote:
>> On 02/06/2012 03:56 PM, [email protected] wrote:
>>>
>>> +enum ath6kl_wow_state {
>>> + ATH6KL_WOW_STATE_NONE,
>>> + ATH6KL_WOW_STATE_SUSPENDING,
>>> + ATH6KL_WOW_STATE_SUSPENDED,
>>> +};
>>> +
>>> struct ath6kl {
>>> struct device *dev;
>>> struct wiphy *wiphy;
>>>
>>> enum ath6kl_state state;
>>> + enum ath6kl_wow_state wow_state;
>>> unsigned int testmode;
>>
>> To be honest, adding a new state variable scares me. I don't see how we
>> are able to maintain two different state variables, the end result would
>> be a total mess.
>
> ath6kl_wow_state is a just sub state of WOW. It roles over only in WOW
> mode. However i understand your point.
It's not just a substate as it's separately checked in txrx.c.
>> I recommend to look at this problem by adding a new state to enum
>> ath6kl_state. That would make it a lot easier to handle all the
>> different states.
>
> The condition to stop ctrl and data pkt transfer are different.
> Ctrl pkt should be stopped when wow_suspended (after sending
> set_host_sleep_cmd_mode). Data pkt should be dropped before
> the moment we configure set_ip_cmd().
Don't think about dropping packets, instead try to make sure that the
packet flow is _stopped_. Dropping packets should be the last resort,
instead we need to go to the source of the packets. For data packets
this means that we need to stop netdev queues. For control packets we
need to make sure that cfg80211 ops in cfg80211.c don't issue any new
wmi commands. And maybe we should also consider debugfs and wmi events
as they can also issue new wmi commands.
> This where we need a state WOW_SUSPENDING and WOW_SUSPENDED.
> enum ath6kl_state has over all ath6kl suspend state (cut pwr, deep
> sleep,wow). IMHO, mixing WOW sub states there is not good approach.
How is this problem related only to WoW? Doesn't it apply to all suspend
modes?
> If you feel maintaining separate state is not good idea, i could think
> of introducing two new flag WOW_SUSPENDING, WOW_SUSPENDED in ar->flag.
After a quick thought having a SUSPENDING state makes, but I can't see
the reason for name WOW_SUSPENDING. WOW_SUSPENDED is unclear for me,
what's the difference to ATH6KL_STATE_WOW?
Kalle
On 02/06/2012 03:56 PM, [email protected] wrote:
> From: Raja Mani <[email protected]>
>
> * TX operation (ctrl tx and data tx) has to be controlled based on
> WOW suspend state. i.e, control packets are allowed to send from
> the host until the suspend state goes ATH6KL_STATE_WOW and
> the data packets are allowed until WOW suspend operation starts.
>
> * Similary, wow resume is NOT allowed if WOW suspend is in progress.
>
> Both of the above scenarios are taken care in this patch.
>
> Signed-off-by: Raja Mani <[email protected]>
What user visible bug are you fixing here?
> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
> @@ -284,6 +284,10 @@ int ath6kl_control_tx(void *devt, struct sk_buff *skb,
> int status = 0;
> struct ath6kl_cookie *cookie = NULL;
>
> +#ifdef CONFIG_PM
> + if (ar->wow_state == ATH6KL_WOW_STATE_SUSPENDED)
> + return -EACCES;
> +#endif
> spin_lock_bh(&ar->lock);
>
> ath6kl_dbg(ATH6KL_DBG_WLAN_TX,
> @@ -352,7 +356,12 @@ int ath6kl_data_tx(struct sk_buff *skb, struct net_device *dev)
> ath6kl_dbg(ATH6KL_DBG_WLAN_TX,
> "%s: skb=0x%p, data=0x%p, len=0x%x\n", __func__,
> skb, skb->data, skb->len);
> -
> +#ifdef CONFIG_PM
> + if (ar->wow_state != ATH6KL_WOW_STATE_NONE) {
> + dev_kfree_skb(skb);
> + return 0;
> + }
> +#endif
I think we should be more proactive than reactive. For example, we
should stop netdev queues to make sure that there are no data packets
transmitted. And we should do the same for ath6kl_control_tx() callers,
I guess that means cfg80211 ops?
But we could add warnings here to make sure that we are not transmitting
any packets in a wrong state.
Kalle
On 02/06/2012 03:56 PM, [email protected] wrote:
> From: Raja Mani <[email protected]>
>
> * TX operation (ctrl tx and data tx) has to be controlled based on
> WOW suspend state. i.e, control packets are allowed to send from
> the host until the suspend state goes ATH6KL_STATE_WOW and
> the data packets are allowed until WOW suspend operation starts.
>
> * Similary, wow resume is NOT allowed if WOW suspend is in progress.
>
> Both of the above scenarios are taken care in this patch.
>
> Signed-off-by: Raja Mani <[email protected]>
[...]
> +enum ath6kl_wow_state {
> + ATH6KL_WOW_STATE_NONE,
> + ATH6KL_WOW_STATE_SUSPENDING,
> + ATH6KL_WOW_STATE_SUSPENDED,
> +};
> +
> struct ath6kl {
> struct device *dev;
> struct wiphy *wiphy;
>
> enum ath6kl_state state;
> + enum ath6kl_wow_state wow_state;
> unsigned int testmode;
To be honest, adding a new state variable scares me. I don't see how we
are able to maintain two different state variables, the end result would
be a total mess.
I recommend to look at this problem by adding a new state to enum
ath6kl_state. That would make it a lot easier to handle all the
different states.
(I haven't looked rest of your patch yet.)
Kalle
On Monday 06 February 2012 07:44 PM, Kalle Valo wrote:
> On 02/06/2012 03:56 PM, [email protected] wrote:
>> From: Raja Mani<[email protected]>
>>
>> * TX operation (ctrl tx and data tx) has to be controlled based on
>> WOW suspend state. i.e, control packets are allowed to send from
>> the host until the suspend state goes ATH6KL_STATE_WOW and
>> the data packets are allowed until WOW suspend operation starts.
>>
>> * Similary, wow resume is NOT allowed if WOW suspend is in progress.
>>
>> Both of the above scenarios are taken care in this patch.
>>
>> Signed-off-by: Raja Mani<[email protected]>
>
> [...]
>
>> +enum ath6kl_wow_state {
>> + ATH6KL_WOW_STATE_NONE,
>> + ATH6KL_WOW_STATE_SUSPENDING,
>> + ATH6KL_WOW_STATE_SUSPENDED,
>> +};
>> +
>> struct ath6kl {
>> struct device *dev;
>> struct wiphy *wiphy;
>>
>> enum ath6kl_state state;
>> + enum ath6kl_wow_state wow_state;
>> unsigned int testmode;
>
> To be honest, adding a new state variable scares me. I don't see how we
> are able to maintain two different state variables, the end result would
> be a total mess.
ath6kl_wow_state is a just sub state of WOW. It roles over only in WOW
mode. However i understand your point.
>
> I recommend to look at this problem by adding a new state to enum
> ath6kl_state. That would make it a lot easier to handle all the
> different states.
The condition to stop ctrl and data pkt transfer are different.
Ctrl pkt should be stopped when wow_suspended (after sending
set_host_sleep_cmd_mode). Data pkt should be dropped before
the moment we configure set_ip_cmd().
This where we need a state WOW_SUSPENDING and WOW_SUSPENDED.
enum ath6kl_state has over all ath6kl suspend state (cut pwr, deep
sleep,wow). IMHO, mixing WOW sub states there is not good approach.
If you feel maintaining separate state is not good idea, i could think
of introducing two new flag WOW_SUSPENDING, WOW_SUSPENDED in ar->flag.
I may be wrong, What do you say?
>
> (I haven't looked rest of your patch yet.)
>
> Kalle