Re: [PATCH] ath6kl: Check wow state before sending control and data pkt

[Raja Mani <rmani@qca.qualcomm.com>]

On Monday 06 February 2012 07:44 PM, Kalle Valo wrote:
> On 02/06/2012 03:56 PM, rmani@qca.qualcomm.com wrote:
>> From: Raja Mani<rmani@qca.qualcomm.com>
>>
>> * 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<rmani@qca.qualcomm.com>
>
> [...]
>
>> +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