WARN on return of ieee80211_if_config

WARN on return of ieee80211_if_config

[reinette chatre]

Hi Johannes,

In 2.6.30 we are starting to see people encountering a WARN when
resuming with HW rfkill enabled. The WARN is printed from
net/mac80211/pm.c:159 in 2.6.30, for current wireless-testing it is 
net/mac80211/util.c:1050.

With that function resuming multiple interfaces I do not know if it will
make sense to return an error code if one fails. So, it seems that
__ieee80211_resume()/ieee80211_reconfig() will always return zero. Even
so, the case where ieee80211_if_config() fails needs to be
accommodated. 

Would something like this make sense?

diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index 0273023..46f2961 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -156,8 +156,11 @@ int __ieee80211_resume(struct ieee80211_hw *hw)
 		case NL80211_IFTYPE_ADHOC:
 		case NL80211_IFTYPE_AP:
 		case NL80211_IFTYPE_MESH_POINT:
-			WARN_ON(ieee80211_if_config(sdata, changed));
-			ieee80211_bss_info_change_notify(sdata, ~0);
+			if (ieee80211_if_config(sdata, changed))
+				printk(KERN_DEBUG "%s: failed to configure interface\n",
+				       sdata->dev->name);
+			else
+				ieee80211_bss_info_change_notify(sdata, ~0);
 			break;
 		case NL80211_IFTYPE_WDS:
 			break;


Thank you

Reinette

Re: WARN on return of ieee80211_if_config

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]

Hi Reinette,

> In 2.6.30 we are starting to see people encountering a WARN when
> resuming with HW rfkill enabled. The WARN is printed from
> net/mac80211/pm.c:159 in 2.6.30, for current wireless-testing it is 
> net/mac80211/util.c:1050.

Hmm, ok, that makes sense.

> With that function resuming multiple interfaces I do not know if it will
> make sense to return an error code if one fails. So, it seems that
> __ieee80211_resume()/ieee80211_reconfig() will always return zero. Even
> so, the case where ieee80211_if_config() fails needs to be
> accommodated. 
> 
> Would something like this make sense?
> 
> diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
> index 0273023..46f2961 100644
> --- a/net/mac80211/pm.c
> +++ b/net/mac80211/pm.c
> @@ -156,8 +156,11 @@ int __ieee80211_resume(struct ieee80211_hw *hw)
>  		case NL80211_IFTYPE_ADHOC:
>  		case NL80211_IFTYPE_AP:
>  		case NL80211_IFTYPE_MESH_POINT:
> -			WARN_ON(ieee80211_if_config(sdata, changed));
> -			ieee80211_bss_info_change_notify(sdata, ~0);
> +			if (ieee80211_if_config(sdata, changed))
> +				printk(KERN_DEBUG "%s: failed to configure interface\n",
> +				       sdata->dev->name);
> +			else
> +				ieee80211_bss_info_change_notify(sdata, ~0);

Not sure. That doesn't seem to make sense anyway, since iwlwifi does
this:

        if (iwl_is_rfkill(priv))
                goto done;
...
 done:
...
	return 0;


Or will this test:
        if (!iwl_is_alive(priv))
                return -EAGAIN;

kick us out before even getting to the rfkill test?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: WARN on return of ieee80211_if_config

[reinette chatre]

Hi Johannes,

On Mon, 2009-04-20 at 11:35 -0700, Johannes Berg wrote:
> Hi Reinette,
> 
> > In 2.6.30 we are starting to see people encountering a WARN when
> > resuming with HW rfkill enabled. The WARN is printed from
> > net/mac80211/pm.c:159 in 2.6.30, for current wireless-testing it is 
> > net/mac80211/util.c:1050.
> 
> Hmm, ok, that makes sense.
> 
> > With that function resuming multiple interfaces I do not know if it will
> > make sense to return an error code if one fails. So, it seems that
> > __ieee80211_resume()/ieee80211_reconfig() will always return zero. Even
> > so, the case where ieee80211_if_config() fails needs to be
> > accommodated. 
> > 
> > Would something like this make sense?
> > 
> > diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
> > index 0273023..46f2961 100644
> > --- a/net/mac80211/pm.c
> > +++ b/net/mac80211/pm.c
> > @@ -156,8 +156,11 @@ int __ieee80211_resume(struct ieee80211_hw *hw)
> >  		case NL80211_IFTYPE_ADHOC:
> >  		case NL80211_IFTYPE_AP:
> >  		case NL80211_IFTYPE_MESH_POINT:
> > -			WARN_ON(ieee80211_if_config(sdata, changed));
> > -			ieee80211_bss_info_change_notify(sdata, ~0);
> > +			if (ieee80211_if_config(sdata, changed))
> > +				printk(KERN_DEBUG "%s: failed to configure interface\n",
> > +				       sdata->dev->name);
> > +			else
> > +				ieee80211_bss_info_change_notify(sdata, ~0);
> 
> Not sure. That doesn't seem to make sense anyway, since iwlwifi does
> this:
> 
>         if (iwl_is_rfkill(priv))
>                 goto done;
> ...
>  done:
> ...
> 	return 0;
> 
> 
> Or will this test:
>         if (!iwl_is_alive(priv))
>                 return -EAGAIN;
> 
> kick us out before even getting to the rfkill test?

Yes ... and it is not immediately clear that it is caused by rfkill. The
STATUS_ALIVE bit that is tested here is cleared when machine is
suspended. When machine is resumed and HW rfkill is set then it will not
come up fully, see __iwl_up() that returns early with success if rfkill
is enabled. STATUS_ALIVE will only be set again after HW rfkill is
disabled and we initialize ucode again.

Reinette

Re: WARN on return of ieee80211_if_config

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Mon, 2009-04-20 at 13:58 -0700, reinette chatre wrote:

> > Or will this test:
> >         if (!iwl_is_alive(priv))
> >                 return -EAGAIN;
> > 
> > kick us out before even getting to the rfkill test?
> 
> Yes ... and it is not immediately clear that it is caused by rfkill. The
> STATUS_ALIVE bit that is tested here is cleared when machine is
> suspended. When machine is resumed and HW rfkill is set then it will not
> come up fully, see __iwl_up() that returns early with success if rfkill
> is enabled. STATUS_ALIVE will only be set again after HW rfkill is
> disabled and we initialize ucode again.

Ok... Very confusing. Can you change the patch to have a comment that
this should also be changed back when rfkill is integrated with
mac80211? Maybe I'll even do that for .31.

Thanks,
Johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]