[PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Mohammed Shafi Shajakhan]

From: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>

In ath9k we make sure the following two things
*if the first interface is ADHOC we cannot have any other interface.
*we cannot add an ADHOC interface if there is already an interface
is present.

when drv_add_interface is called during resume we got to consider
number of vifs already present in addition to checking the drivers
'opmode' information about ADHOC, otherwise during suspend/resume
we incorrectly assume an ADHOC interface is already present.
Then we may miss some driver specific data for the ADHOC
interface after resume.

	ath: phy0: Cannot create ADHOC interface when other
	interfaces already exist.
	WARNING: at net/mac80211/driver-ops.h:12
	ieee80211_reconfig+0x1882/0x1ca0 [mac80211]()
	Hardware name: 2842RK1
	wlan2:  Failed check-sdata-in-driver check, flags: 0x0

	Call Trace:
	[<c01361b2>] warn_slowpath_common+0x72/0xa0
	[<f8aaa7c2>] ? ieee80211_reconfig+0x1882/0x1ca0
	[mac80211]
	[<f8aaa7c2>] ? ieee80211_reconfig+0x1882/0x1ca0
	[mac80211]
	[<c0136283>] warn_slowpath_fmt+0x33/0x40
	[<f8aaa7c2>] ieee80211_reconfig+0x1882/0x1ca0 [mac80211]
	[<c06c1d1a>] ? mutex_lock_nested+0x23a/0x2f0
	[<f8a95097>] ieee80211_resume+0x27/0x70 [mac80211]
	[<fd177edf>] wiphy_resume+0x8f/0xa0 [cfg80211]

Cc: stable@vger.kernel.org
Cc: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/main.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 4de4473..c26497d 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1443,11 +1443,14 @@ static int ath9k_add_interface(struct ieee80211_hw *hw,
 		}
 	}
 
-	if ((ah->opmode == NL80211_IFTYPE_ADHOC) ||
-	    ((vif->type == NL80211_IFTYPE_ADHOC) &&
-	     sc->nvifs > 0)) {
-		ath_err(common, "Cannot create ADHOC interface when other"
-			" interfaces already exist.\n");
+	if ((ah->opmode == NL80211_IFTYPE_ADHOC) && (sc->nvifs > 0)) {
+		ath_err(common, "Cannot create any other interface when an ADHOC interface already exists.\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if ((vif->type == NL80211_IFTYPE_ADHOC) && (sc->nvifs > 0)) {
+		ath_err(common, "Cannot create ADHOC interface when other interfaces already exist.\n");
 		ret = -EINVAL;
 		goto out;
 	}
-- 
1.7.0.4

Re: [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Johannes Berg]

On Fri, 2012-06-01 at 12:09 +0530, Mohammed Shafi Shajakhan wrote:
> From: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>
> 
> In ath9k we make sure the following two things
> *if the first interface is ADHOC we cannot have any other interface.
> *we cannot add an ADHOC interface if there is already an interface
> is present.

> -	if ((ah->opmode == NL80211_IFTYPE_ADHOC) ||
> -	    ((vif->type == NL80211_IFTYPE_ADHOC) &&
> -	     sc->nvifs > 0)) {
> -		ath_err(common, "Cannot create ADHOC interface when other"
> -			" interfaces already exist.\n");
> +	if ((ah->opmode == NL80211_IFTYPE_ADHOC) && (sc->nvifs > 0)) {
> +		ath_err(common, "Cannot create any other interface when an ADHOC interface already exists.\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if ((vif->type == NL80211_IFTYPE_ADHOC) && (sc->nvifs > 0)) {
> +		ath_err(common, "Cannot create ADHOC interface when other interfaces already exist.\n");

You could just remove the entire check since the interface combinations
you advertise don't allow it, I think? Or just fix those
combinations :-)

johannes

Re: [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Mohammed Shafi Shajakhan]

Hi Johannes,

On Friday 01 June 2012 12:14 PM, Johannes Berg wrote:
> On Fri, 2012-06-01 at 12:09 +0530, Mohammed Shafi Shajakhan wrote:
>> From: Mohammed Shafi Shajakhan<mohammed@qca.qualcomm.com>
>>
>> In ath9k we make sure the following two things
>> *if the first interface is ADHOC we cannot have any other interface.
>> *we cannot add an ADHOC interface if there is already an interface
>> is present.
>
>> -	if ((ah->opmode == NL80211_IFTYPE_ADHOC) ||
>> -	    ((vif->type == NL80211_IFTYPE_ADHOC)&&
>> -	     sc->nvifs>  0)) {
>> -		ath_err(common, "Cannot create ADHOC interface when other"
>> -			" interfaces already exist.\n");
>> +	if ((ah->opmode == NL80211_IFTYPE_ADHOC)&&  (sc->nvifs>  0)) {
>> +		ath_err(common, "Cannot create any other interface when an ADHOC interface already exists.\n");
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	if ((vif->type == NL80211_IFTYPE_ADHOC)&&  (sc->nvifs>  0)) {
>> +		ath_err(common, "Cannot create ADHOC interface when other interfaces already exist.\n");
>
> You could just remove the entire check since the interface combinations
> you advertise don't allow it, I think? Or just fix those
> combinations :-)

i did not check this before, thanks a lot for your inputs. will send a 
proper v2 after checking this out.


-- 
thanks,
shafi

Re: [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Johannes Berg]

On Fri, 2012-06-01 at 12:39 +0530, Mohammed Shafi Shajakhan wrote:
> Hi Johannes,
> 
> On Friday 01 June 2012 12:14 PM, Johannes Berg wrote:
> > On Fri, 2012-06-01 at 12:09 +0530, Mohammed Shafi Shajakhan wrote:
> >> From: Mohammed Shafi Shajakhan<mohammed@qca.qualcomm.com>
> >>
> >> In ath9k we make sure the following two things
> >> *if the first interface is ADHOC we cannot have any other interface.
> >> *we cannot add an ADHOC interface if there is already an interface
> >> is present.
> >
> >> -	if ((ah->opmode == NL80211_IFTYPE_ADHOC) ||
> >> -	    ((vif->type == NL80211_IFTYPE_ADHOC)&&
> >> -	     sc->nvifs>  0)) {
> >> -		ath_err(common, "Cannot create ADHOC interface when other"
> >> -			" interfaces already exist.\n");
> >> +	if ((ah->opmode == NL80211_IFTYPE_ADHOC)&&  (sc->nvifs>  0)) {
> >> +		ath_err(common, "Cannot create any other interface when an ADHOC interface already exists.\n");
> >> +		ret = -EINVAL;
> >> +		goto out;
> >> +	}
> >> +
> >> +	if ((vif->type == NL80211_IFTYPE_ADHOC)&&  (sc->nvifs>  0)) {
> >> +		ath_err(common, "Cannot create ADHOC interface when other interfaces already exist.\n");
> >
> > You could just remove the entire check since the interface combinations
> > you advertise don't allow it, I think? Or just fix those
> > combinations :-)
> 
> i did not check this before, thanks a lot for your inputs. will send a 
> proper v2 after checking this out.

If this is needed for stable, you might want to keep this patch & send
another one to remove it.

johannes

Re: [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Mohammed Shafi Shajakhan]

On Friday 01 June 2012 12:43 PM, Johannes Berg wrote:
> On Fri, 2012-06-01 at 12:39 +0530, Mohammed Shafi Shajakhan wrote:
>> Hi Johannes,
>>
>> On Friday 01 June 2012 12:14 PM, Johannes Berg wrote:
>>> On Fri, 2012-06-01 at 12:09 +0530, Mohammed Shafi Shajakhan wrote:
>>>> From: Mohammed Shafi Shajakhan<mohammed@qca.qualcomm.com>
>>>>
>>>> In ath9k we make sure the following two things
>>>> *if the first interface is ADHOC we cannot have any other interface.
>>>> *we cannot add an ADHOC interface if there is already an interface
>>>> is present.
>>>
>>>> -	if ((ah->opmode == NL80211_IFTYPE_ADHOC) ||
>>>> -	    ((vif->type == NL80211_IFTYPE_ADHOC)&&
>>>> -	     sc->nvifs>   0)) {
>>>> -		ath_err(common, "Cannot create ADHOC interface when other"
>>>> -			" interfaces already exist.\n");
>>>> +	if ((ah->opmode == NL80211_IFTYPE_ADHOC)&&   (sc->nvifs>   0)) {
>>>> +		ath_err(common, "Cannot create any other interface when an ADHOC interface already exists.\n");
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	if ((vif->type == NL80211_IFTYPE_ADHOC)&&   (sc->nvifs>   0)) {
>>>> +		ath_err(common, "Cannot create ADHOC interface when other interfaces already exist.\n");
>>>
>>> You could just remove the entire check since the interface combinations
>>> you advertise don't allow it, I think? Or just fix those
>>> combinations :-)
>>
>> i did not check this before, thanks a lot for your inputs. will send a
>> proper v2 after checking this out.
>
> If this is needed for stable, you might want to keep this patch&  send
> another one to remove it.

thanks Johannes.
i was looking at how to fix this properly in iface combination 
advertised to mac80211.  got few doubts regarding this

*if an interface type is not all advertised in the 
ieee80211_iface_combination then it cannot it be co-existing with any 
other interfaces ?
please let me know is there some other way do that.

if we advertise something like this

static const struct ieee80211_iface_limit if_limits[] = {
	{set1
     	.... },
	{set 2
	.... },
};

then interface types in set1 and set2 co-exist as per the logic in 
cfg80211_can_change_interface. is there already a way we can make
set1 and set2 interface types mutually exclusive ? thanks!



-- 
thanks,
shafi

Re: [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Johannes Berg]

Hi Mohammed,

> >>> You could just remove the entire check since the interface combinations
> >>> you advertise don't allow it, I think? Or just fix those
> >>> combinations :-)
> >>
> >> i did not check this before, thanks a lot for your inputs. will send a
> >> proper v2 after checking this out.
> >
> > If this is needed for stable, you might want to keep this patch&  send
> > another one to remove it.
> 
> thanks Johannes.
> i was looking at how to fix this properly in iface combination 
> advertised to mac80211.  got few doubts regarding this
> 
> *if an interface type is not all advertised in the 
> ieee80211_iface_combination then it cannot it be co-existing with any 
> other interfaces ?
> please let me know is there some other way do that.
> 
> if we advertise something like this
> 
> static const struct ieee80211_iface_limit if_limits[] = {
> 	{set1
>      	.... },
> 	{set 2
> 	.... },
> };
> 
> then interface types in set1 and set2 co-exist as per the logic in 
> cfg80211_can_change_interface. is there already a way we can make
> set1 and set2 interface types mutually exclusive ? thanks!

The sets are mutually exclusive, and there are implied sets of each
interface with a max number of 1. So for example, in iwlwifi we don't
advertise IBSS in the combinations at all, because it's not compatible
with anything. In your case, I think the same applies, since you said
        
        if the first interface is ADHOC we cannot have any other
        interface. we cannot add an ADHOC interface if there is already
        an interface is present

Thus, if you leave IBSS out of the combinations you should get the
desired behaviour of not being able to combine IBSS with any other
types.

johannes

Re: [ath9k-devel] [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Guido Iribarren]

On Sat, Jun 2, 2012 at 2:45 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> The sets are mutually exclusive, and there are implied sets of each
> interface with a max number of 1. So for example, in iwlwifi we don't
> advertise IBSS in the combinations at all, because it's not compatible
> with anything. In your case, I think the same applies, since you said
>
>        if the first interface is ADHOC we cannot have any other
>        interface. we cannot add an ADHOC interface if there is already
>        an interface is present

I've been following the thread and still can't get this confusion out
of my head:
i'm currently using kernel 3.3.2 (openwrt trunk r31439), with Atheros
AR9285 and i can succesfully combine IBSS interface with AP or STA
modes...

# iw dev
phy#0
	Interface wlan0-1
		ifindex 11
		type AP
	Interface wlan0-2
		ifindex 10
		type IBSS
	Interface wlan0
		ifindex 9
		type AP

So, does this patch mean this functionality will be lost in future
versions of ath9k?
or maybe it is officially unsupported and openwrt has patches applied?
or, even worse, I completely misunderstood what you're talking about?
(in that case i'm sorry for the noise)

Thanks a lot,

Guido

Re: [ath9k-devel] [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Felix Fietkau]

On 2012-06-02 8:14 PM, Guido Iribarren wrote:
> On Sat, Jun 2, 2012 at 2:45 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>> The sets are mutually exclusive, and there are implied sets of each
>> interface with a max number of 1. So for example, in iwlwifi we don't
>> advertise IBSS in the combinations at all, because it's not compatible
>> with anything. In your case, I think the same applies, since you said
>>
>>        if the first interface is ADHOC we cannot have any other
>>        interface. we cannot add an ADHOC interface if there is already
>>        an interface is present
> 
> I've been following the thread and still can't get this confusion out
> of my head:
> i'm currently using kernel 3.3.2 (openwrt trunk r31439), with Atheros
> AR9285 and i can succesfully combine IBSS interface with AP or STA
> modes...
> 
> # iw dev
> phy#0
> 	Interface wlan0-1
> 		ifindex 11
> 		type AP
> 	Interface wlan0-2
> 		ifindex 10
> 		type IBSS
> 	Interface wlan0
> 		ifindex 9
> 		type AP
> 
> So, does this patch mean this functionality will be lost in future
> versions of ath9k?
> or maybe it is officially unsupported and openwrt has patches applied?
> or, even worse, I completely misunderstood what you're talking about?
> (in that case i'm sorry for the noise)
It's officially unsupported, OpenWrt has patches to enable it.

- Felix

Re: [PATCH] ath9k: Fix a WARNING in suspend/resume with IBSS

[Mohammed Shafi Shajakhan]

Hi Johannes,


>>>>> You could just remove the entire check since the interface combinations
>>>>> you advertise don't allow it, I think? Or just fix those
>>>>> combinations :-)
>>>>
>>>> i did not check this before, thanks a lot for your inputs. will send a
>>>> proper v2 after checking this out.
>>>
>>> If this is needed for stable, you might want to keep this patch&   send
>>> another one to remove it.
>>
>> thanks Johannes.
>> i was looking at how to fix this properly in iface combination
>> advertised to mac80211.  got few doubts regarding this
>>
>> *if an interface type is not all advertised in the
>> ieee80211_iface_combination then it cannot it be co-existing with any
>> other interfaces ?
>> please let me know is there some other way do that.
>>
>> if we advertise something like this
>>
>> static const struct ieee80211_iface_limit if_limits[] = {
>> 	{set1
>>       	.... },
>> 	{set 2
>> 	.... },
>> };
>>
>> then interface types in set1 and set2 co-exist as per the logic in
>> cfg80211_can_change_interface. is there already a way we can make
>> set1 and set2 interface types mutually exclusive ? thanks!
>
> The sets are mutually exclusive, and there are implied sets of each
> interface with a max number of 1. So for example, in iwlwifi we don't
> advertise IBSS in the combinations at all, because it's not compatible
> with anything. In your case, I think the same applies, since you said
>
>          if the first interface is ADHOC we cannot have any other
>          interface. we cannot add an ADHOC interface if there is already
>          an interface is present
>
> Thus, if you leave IBSS out of the combinations you should get the
> desired behaviour of not being able to combine IBSS with any other
> types.
>

ath9k and also iwlwifi seems to have IBSS check not included in any of 
the interface combinations, but still i am able to IBSS interface 
atleast in ath9k with the checks removed in drv_add_interface.
it seems we are allowing any interface type to be added even if
its not added in the ieee80211_iface_combination structure.

i am sending an RFC to add this check in cfg80211 based on my 
understanding. please review my understanding and any corrections
needed. thanks for your thoughts!

-- 
thanks,
shafi