2009-03-26 17:08:42

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 0/3] iwlwifi driver updates 03/26/2009

In this series we improve support for suspend/resume by making use of
infrastructure already in place in mac80211. We fix the problem where
device cannot come back up if rfkill was enabled during ifdown. The 3945
is a porting patch but through this also fixes a locking issue that was
present in iwl3945 but not in iwlagn (see
https://bugzilla.redhat.com/show_bug.cgi?id=492116).

[PATCH 1/3] iwlwifi: merge and better support of suspend/resume for iwlagn and iwl3945
[PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill
[PATCH 3/3] iwl3945: use iwl_mac_conf_tx


Thank you

Reinette



2009-03-26 17:50:21

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Hi,

Am Donnerstag, 26. M=E4rz 2009 schrieb Reinette Chatre:
> From: Wey-Yi Guy <[email protected]>
>=20
> Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit get
> clear in __iwl_down() function; the additional checking will fail and
> cause RF can not be turn back on.

Are you sure this is needed? I'd argue we should only restart the adapt=
er
if it was alive when it got rf_killed. In case the adapter was rf_kille=
d
while the interface was down I don't think we want to restart the adapt=
er
immediately but first when the interface is taken up again.

Helmut

> Signed-off-by: Wey-Yi Guy <[email protected]>
> Signed-off-by: Reinette Chatre <[email protected]>
> ---
> drivers/net/wireless/iwlwifi/iwl-core.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wi=
reless/iwlwifi/iwl-core.c
> index e1b5472..7560d6f 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-core.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-core.c
> @@ -2056,8 +2056,7 @@ void iwl_bg_rf_kill(struct work_struct *work)
> IWL_DEBUG_RF_KILL(priv,
> "HW and/or SW RF Kill no longer active, restarting "
> "device\n");
> - if (!test_bit(STATUS_EXIT_PENDING, &priv->status) &&
> - test_bit(STATUS_ALIVE, &priv->status))
> + if (!test_bit(STATUS_EXIT_PENDING, &priv->status))
> queue_work(priv->workqueue, &priv->restart);
> } else {
> /* make sure mac80211 stop sending Tx frame */

2009-03-26 19:49:29

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> On Thu, 2009-03-26 at 12:11 -0700, Helmut Schaa wrote:
> > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> > > > Hi,
> > > >=20
> > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > > > > From: Wey-Yi Guy <[email protected]>
> > > > >=20
> > > > > Remove STATUS_ALIVE checking when HW RF KILL disabled, the bi=
t get
> > > > > clear in __iwl_down() function; the additional checking will =
fail and
> > > > > cause RF can not be turn back on.
> > > >=20
> > > > Are you sure this is needed? I'd argue we should only restart t=
he adapter
> > > > if it was alive when it got rf_killed. In case the adapter was =
rf_killed
> > > > while the interface was down I don't think we want to restart t=
he adapter
> > > > immediately but first when the interface is taken up again.
> > >=20
> > > We also need to consider if a suspend/resume happens in the middl=
e.
> > > Without the patch, if you enable rfkill, suspend, resume, disable
> > > rfkill, then your interface cannot be brought up.
> >=20
> > I guess you refer to the situation where the interface is up, right=
?
> > Something like:
> >=20
> > - ifconfig wlan0 up
> > - press killswitch (kill wireless)
> > - suspend
> > - resume
> > - press killswitch (enable wireless)
> > - here the interface should still be up
> >=20
> > As the interface is/was up, mac80211's resume handler should restar=
t the
> > adapter and thus we wouldn't need to restart the adapter in the
> > rfkill-handler, or did I miss anything?
>=20
> Yes, the resume handler will start the adapter (call "start"), but th=
e
> actions done by it will exit early because of rfkill being enabled. T=
he
> STATUS_ALIVE bit will thus not be set after this is completed. Later,
> when user disables rfkill, we want to restart the adapter to get all
> this corrected, but this call currently fails because of this check.

Got it, thanks for the explanation.

Nevertheless, removing the check will result in restarting the adapter =
even
if the interface is down. So, I agree that we have a problem here but I=
do
not agree with the solution ;)

Maybe taking the interface up (not only pseudo-up, as done currently) s=
hould
be allowed even if wireless is killed? We already allow the interface t=
o
stay up when the adapter gets rfkilled.

Helmut

2009-03-27 06:18:29

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Hi again,

Am Donnerstag, 26. M=E4rz 2009 schrieb Reinette Chatre:
> Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit get
> clear in __iwl_down() function; the additional checking will fail and
> cause RF can not be turn back on.

Maybe just checking for is_open instead of STATUS_ALIVE will fix the is=
sue
too? I only had a quick look but it looks possible. AFAICS is_open indi=
cates
if the interface was taken up by mac80211. So if the interface is up bu=
t
STATUS_ALIVE is not set we want to restart the adapter after unkilling =
but
if the interface is still down (is_open=3D=3D0) we do not want to resta=
rt the
adapter. Furthermore, is_open is also set in case the interface was tak=
en
up but wireless was rfkilled.

Helmut

2009-03-26 19:25:31

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

On Thu, 2009-03-26 at 12:11 -0700, Helmut Schaa wrote:
> Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> > > Hi,
> > >=20
> > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > > > From: Wey-Yi Guy <[email protected]>
> > > >=20
> > > > Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit =
get
> > > > clear in __iwl_down() function; the additional checking will fa=
il and
> > > > cause RF can not be turn back on.
> > >=20
> > > Are you sure this is needed? I'd argue we should only restart the=
adapter
> > > if it was alive when it got rf_killed. In case the adapter was rf=
_killed
> > > while the interface was down I don't think we want to restart the=
adapter
> > > immediately but first when the interface is taken up again.
> >=20
> > We also need to consider if a suspend/resume happens in the middle.
> > Without the patch, if you enable rfkill, suspend, resume, disable
> > rfkill, then your interface cannot be brought up.
>=20
> I guess you refer to the situation where the interface is up, right?
> Something like:
>=20
> - ifconfig wlan0 up
> - press killswitch (kill wireless)
> - suspend
> - resume
> - press killswitch (enable wireless)
> - here the interface should still be up
>=20
> As the interface is/was up, mac80211's resume handler should restart =
the
> adapter and thus we wouldn't need to restart the adapter in the
> rfkill-handler, or did I miss anything?

Yes, the resume handler will start the adapter (call "start"), but the
actions done by it will exit early because of rfkill being enabled. The
STATUS_ALIVE bit will thus not be set after this is completed. Later,
when user disables rfkill, we want to restart the adapter to get all
this corrected, but this call currently fails because of this check.

Reinette

2009-03-26 19:12:03

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> > Hi,
> >=20
> > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > > From: Wey-Yi Guy <[email protected]>
> > >=20
> > > Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit ge=
t
> > > clear in __iwl_down() function; the additional checking will fail=
and
> > > cause RF can not be turn back on.
> >=20
> > Are you sure this is needed? I'd argue we should only restart the a=
dapter
> > if it was alive when it got rf_killed. In case the adapter was rf_k=
illed
> > while the interface was down I don't think we want to restart the a=
dapter
> > immediately but first when the interface is taken up again.
>=20
> We also need to consider if a suspend/resume happens in the middle.
> Without the patch, if you enable rfkill, suspend, resume, disable
> rfkill, then your interface cannot be brought up.

I guess you refer to the situation where the interface is up, right?
Something like:

- ifconfig wlan0 up
- press killswitch (kill wireless)
- suspend
- resume
- press killswitch (enable wireless)
- here the interface should still be up

As the interface is/was up, mac80211's resume handler should restart th=
e
adapter and thus we wouldn't need to restart the adapter in the
rfkill-handler, or did I miss anything?

Thanks,
Helmut

2009-03-27 06:25:46

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Hi,

Am Freitag, 27. M=C3=A4rz 2009 schrieb reinette chatre:
> On Thu, 2009-03-26 at 13:58 -0700, Helmut Schaa wrote:
> > Assume the following situation:
> >=20
> > You only have one killswitch for both, wireless and bluetooth. The =
wireless
> > interface is down because it is unused and the user wants to use bl=
uetooth
> > and enables it via the killswitch which also means that wireless ge=
ts
> > unkilled. Now restarting the adapter needs more power then keeping =
the
> > adapter down. In short: if the interface is down the user (space) d=
oes not
> > want to use the interface and probably wants to save power as well.
>=20
> Your measurements are very fine grained :) Will this be measureable?

Heh, you know the internals of the devive ;) but I guess waking up the
adapter (and thus starting the firmware) even if the interface is unuse=
d
will surely need more power than not doing so.

Helmut

2009-03-27 06:19:24

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Am Freitag, 27. M=C3=A4rz 2009 schrieb reinette chatre:
> On Thu, 2009-03-26 at 13:58 -0700, Helmut Schaa wrote:
> > 2) take the interface down when it gets rfkilled
> >=20
> > However, I'm not sure if 2 is possible without help of mac80211 and=
if it
> > makes sense to change an interface state from within the driver.
>=20
> Like you say ... probably not the driver's place to do this.

Yeah, just ignore that idea ;)

Helmut

2009-03-26 23:21:23

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Hi Helmut,

On Thu, 2009-03-26 at 13:58 -0700, Helmut Schaa wrote:
> Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > On Thu, 2009-03-26 at 12:49 -0700, Helmut Schaa wrote:
> > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > > On Thu, 2009-03-26 at 12:11 -0700, Helmut Schaa wrote:
> > > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > > > > On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> > > > > > > Hi,
> > > > > > >=20
> > > > > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre=
:
> > > > > > > > From: Wey-Yi Guy <[email protected]>
> > > > > > > >=20
> > > > > > > > Remove STATUS_ALIVE checking when HW RF KILL disabled, =
the bit get
> > > > > > > > clear in __iwl_down() function; the additional checking=
will fail and
> > > > > > > > cause RF can not be turn back on.
> > > > > > >=20
> > > > > > > Are you sure this is needed? I'd argue we should only res=
tart the adapter
> > > > > > > if it was alive when it got rf_killed. In case the adapte=
r was rf_killed
> > > > > > > while the interface was down I don't think we want to res=
tart the adapter
> > > > > > > immediately but first when the interface is taken up agai=
n.
> > > > > >=20
> > > > > > We also need to consider if a suspend/resume happens in the=
middle.
> > > > > > Without the patch, if you enable rfkill, suspend, resume, d=
isable
> > > > > > rfkill, then your interface cannot be brought up.
> > > > >=20
> > > > > I guess you refer to the situation where the interface is up,=
right?
> > > > > Something like:
> > > > >=20
> > > > > - ifconfig wlan0 up
> > > > > - press killswitch (kill wireless)
> > > > > - suspend
> > > > > - resume
> > > > > - press killswitch (enable wireless)
> > > > > - here the interface should still be up
> > > > >=20
> > > > > As the interface is/was up, mac80211's resume handler should =
restart the
> > > > > adapter and thus we wouldn't need to restart the adapter in t=
he
> > > > > rfkill-handler, or did I miss anything?
> > > >=20
> > > > Yes, the resume handler will start the adapter (call "start"), =
but the
> > > > actions done by it will exit early because of rfkill being enab=
led. The
> > > > STATUS_ALIVE bit will thus not be set after this is completed. =
Later,
> > > > when user disables rfkill, we want to restart the adapter to ge=
t all
> > > > this corrected, but this call currently fails because of this c=
heck.
> > >=20
> > > Got it, thanks for the explanation.
> > >=20
> > > Nevertheless, removing the check will result in restarting the ad=
apter even
> > > if the interface is down. So, I agree that we have a problem here=
but I do
> > > not agree with the solution ;)
> >=20
> > I agree that it is not efficient ... but it seems harmless.
>=20
> Assume the following situation:
>=20
> You only have one killswitch for both, wireless and bluetooth. The wi=
reless
> interface is down because it is unused and the user wants to use blue=
tooth
> and enables it via the killswitch which also means that wireless gets
> unkilled. Now restarting the adapter needs more power then keeping th=
e
> adapter down. In short: if the interface is down the user (space) doe=
s not
> want to use the interface and probably wants to save power as well.

Your measurements are very fine grained :) Will this be measureable?
Right now things do not work and this patch fixes it.=20

> > > Maybe taking the interface up (not only pseudo-up, as done curren=
tly) should
> > > be allowed even if wireless is killed? We already allow the inter=
face to
> > > stay up when the adapter gets rfkilled.
> >=20
> > Knowing that rfkill is enabled enables us to save power by not brin=
ging
> > everything up.
>=20
> Yes, but user (space) knows that the wireless card consumes more powe=
r if the
> interface is up. And if you use NM for example it will just take the =
interface
> down when the killswitch gets activated.

> Another solution would be to
> 1) not allow the interface to go up when it is rfkilled, and

We currently do this.

> 2) take the interface down when it gets rfkilled
>=20
> However, I'm not sure if 2 is possible without help of mac80211 and i=
f it
> makes sense to change an interface state from within the driver.

Like you say ... probably not the driver's place to do this.

Reinette

2009-03-26 20:32:12

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

On Thu, 2009-03-26 at 12:49 -0700, Helmut Schaa wrote:
> Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > On Thu, 2009-03-26 at 12:11 -0700, Helmut Schaa wrote:
> > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > > On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> > > > > Hi,
> > > > >=20
> > > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > > > > > From: Wey-Yi Guy <[email protected]>
> > > > > >=20
> > > > > > Remove STATUS_ALIVE checking when HW RF KILL disabled, the =
bit get
> > > > > > clear in __iwl_down() function; the additional checking wil=
l fail and
> > > > > > cause RF can not be turn back on.
> > > > >=20
> > > > > Are you sure this is needed? I'd argue we should only restart=
the adapter
> > > > > if it was alive when it got rf_killed. In case the adapter wa=
s rf_killed
> > > > > while the interface was down I don't think we want to restart=
the adapter
> > > > > immediately but first when the interface is taken up again.
> > > >=20
> > > > We also need to consider if a suspend/resume happens in the mid=
dle.
> > > > Without the patch, if you enable rfkill, suspend, resume, disab=
le
> > > > rfkill, then your interface cannot be brought up.
> > >=20
> > > I guess you refer to the situation where the interface is up, rig=
ht?
> > > Something like:
> > >=20
> > > - ifconfig wlan0 up
> > > - press killswitch (kill wireless)
> > > - suspend
> > > - resume
> > > - press killswitch (enable wireless)
> > > - here the interface should still be up
> > >=20
> > > As the interface is/was up, mac80211's resume handler should rest=
art the
> > > adapter and thus we wouldn't need to restart the adapter in the
> > > rfkill-handler, or did I miss anything?
> >=20
> > Yes, the resume handler will start the adapter (call "start"), but =
the
> > actions done by it will exit early because of rfkill being enabled.=
The
> > STATUS_ALIVE bit will thus not be set after this is completed. Late=
r,
> > when user disables rfkill, we want to restart the adapter to get al=
l
> > this corrected, but this call currently fails because of this check=
=2E
>=20
> Got it, thanks for the explanation.
>=20
> Nevertheless, removing the check will result in restarting the adapte=
r even
> if the interface is down. So, I agree that we have a problem here but=
I do
> not agree with the solution ;)

I agree that it is not efficient ... but it seems harmless.

> Maybe taking the interface up (not only pseudo-up, as done currently)=
should
> be allowed even if wireless is killed? We already allow the interface=
to
> stay up when the adapter gets rfkilled.

Knowing that rfkill is enabled enables us to save power by not bringing
everything up.

Reinette

2009-03-27 15:40:15

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Hi Helmut,

On Thu, 2009-03-26 at 23:25 -0700, Helmut Schaa wrote:
> Hi,
>=20
> Am Freitag, 27. M=C3=A4rz 2009 schrieb reinette chatre:
> > On Thu, 2009-03-26 at 13:58 -0700, Helmut Schaa wrote:
> > > Assume the following situation:
> > >=20
> > > You only have one killswitch for both, wireless and bluetooth. Th=
e wireless
> > > interface is down because it is unused and the user wants to use =
bluetooth
> > > and enables it via the killswitch which also means that wireless =
gets
> > > unkilled. Now restarting the adapter needs more power then keepin=
g the
> > > adapter down. In short: if the interface is down the user (space)=
does not
> > > want to use the interface and probably wants to save power as wel=
l.
> >=20
> > Your measurements are very fine grained :) Will this be measureable=
?
>=20
> Heh, you know the internals of the devive ;) but I guess waking up th=
e
> adapter (and thus starting the firmware) even if the interface is unu=
sed
> will surely need more power than not doing so.

If interface is down then mac will not ask us to start the interface in
the first place. If interface is up and mac asks us to start interface
then the driver will do so. In this case, if rfkill is enabled, then th=
e
driver will not do much ... definitely not load firmware and thus start
using up power. Only when the user changes rfkill state will we actuall=
y
get the firmware going.

Reinette

2009-03-27 03:44:27

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Hi Helmut,

On Thu, 2009-03-26 at 13:58 -0700, Helmut Schaa wrote:
> Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > On Thu, 2009-03-26 at 12:49 -0700, Helmut Schaa wrote:
> > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > > On Thu, 2009-03-26 at 12:11 -0700, Helmut Schaa wrote:
> > > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > > > > On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> > > > > > > Hi,
> > > > > > >=20
> > > > > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre=
:
> > > > > > > > From: Wey-Yi Guy <[email protected]>
> > > > > > > >=20
> > > > > > > > Remove STATUS_ALIVE checking when HW RF KILL disabled, =
the bit get
> > > > > > > > clear in __iwl_down() function; the additional checking=
will fail and
> > > > > > > > cause RF can not be turn back on.
> > > > > > >=20
> > > > > > > Are you sure this is needed? I'd argue we should only res=
tart the adapter
> > > > > > > if it was alive when it got rf_killed. In case the adapte=
r was rf_killed
> > > > > > > while the interface was down I don't think we want to res=
tart the adapter
> > > > > > > immediately but first when the interface is taken up agai=
n.
> > > > > >=20
> > > > > > We also need to consider if a suspend/resume happens in the=
middle.
> > > > > > Without the patch, if you enable rfkill, suspend, resume, d=
isable
> > > > > > rfkill, then your interface cannot be brought up.
> > > > >=20
> > > > > I guess you refer to the situation where the interface is up,=
right?
> > > > > Something like:
> > > > >=20
> > > > > - ifconfig wlan0 up
> > > > > - press killswitch (kill wireless)
> > > > > - suspend
> > > > > - resume
> > > > > - press killswitch (enable wireless)
> > > > > - here the interface should still be up
> > > > >=20
> > > > > As the interface is/was up, mac80211's resume handler should =
restart the
> > > > > adapter and thus we wouldn't need to restart the adapter in t=
he
> > > > > rfkill-handler, or did I miss anything?
> > > >=20
> > > > Yes, the resume handler will start the adapter (call "start"), =
but the
> > > > actions done by it will exit early because of rfkill being enab=
led. The
> > > > STATUS_ALIVE bit will thus not be set after this is completed. =
Later,
> > > > when user disables rfkill, we want to restart the adapter to ge=
t all
> > > > this corrected, but this call currently fails because of this c=
heck.
> > >=20
> > > Got it, thanks for the explanation.
> > >=20
> > > Nevertheless, removing the check will result in restarting the ad=
apter even
> > > if the interface is down. So, I agree that we have a problem here=
but I do
> > > not agree with the solution ;)
> >=20
> > I agree that it is not efficient ... but it seems harmless.
>=20
> Assume the following situation:
>=20
> You only have one killswitch for both, wireless and bluetooth. The wi=
reless
> interface is down because it is unused and the user wants to use blue=
tooth
> and enables it via the killswitch which also means that wireless gets
> unkilled. Now restarting the adapter needs more power then keeping th=
e
> adapter down. In short: if the interface is down the user (space) doe=
s not
> want to use the interface and probably wants to save power as well.
>=20
> > > Maybe taking the interface up (not only pseudo-up, as done curren=
tly) should
> > > be allowed even if wireless is killed? We already allow the inter=
face to
> > > stay up when the adapter gets rfkilled.
> >=20
> > Knowing that rfkill is enabled enables us to save power by not brin=
ging
> > everything up.
>=20
> Yes, but user (space) knows that the wireless card consumes more powe=
r if the
> interface is up. And if you use NM for example it will just take the =
interface
> down when the killswitch gets activated.
>=20
> Another solution would be to
> 1) not allow the interface to go up when it is rfkilled, and
> 2) take the interface down when it gets rfkilled
>=20
> However, I'm not sure if 2 is possible without help of mac80211 and i=
f it
> makes sense to change an interface state from within the driver.

I was just thinking that some of this thread can be interpreted
incorrectly. There is a problem with shared information here ...
mac80211 is what brings the interface up and down, yet it is the driver
that has the knowledge about rfkill state. There has been talk about
having rfkill support in mac80211, but it is not there at this time.
Until it is, the driver needs to take care that interface is not fully
up (firmware running full throttle) if rfkill enabled, yet let mac80211
think that interface up was successful. Then, when rfkill is disabled w=
e
can complete interface bringup.

Reinette=20

2009-03-26 20:59:05

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> On Thu, 2009-03-26 at 12:49 -0700, Helmut Schaa wrote:
> > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > On Thu, 2009-03-26 at 12:11 -0700, Helmut Schaa wrote:
> > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb reinette chatre:
> > > > > On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> > > > > > Hi,
> > > > > >=20
> > > > > > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > > > > > > From: Wey-Yi Guy <[email protected]>
> > > > > > >=20
> > > > > > > Remove STATUS_ALIVE checking when HW RF KILL disabled, th=
e bit get
> > > > > > > clear in __iwl_down() function; the additional checking w=
ill fail and
> > > > > > > cause RF can not be turn back on.
> > > > > >=20
> > > > > > Are you sure this is needed? I'd argue we should only resta=
rt the adapter
> > > > > > if it was alive when it got rf_killed. In case the adapter =
was rf_killed
> > > > > > while the interface was down I don't think we want to resta=
rt the adapter
> > > > > > immediately but first when the interface is taken up again.
> > > > >=20
> > > > > We also need to consider if a suspend/resume happens in the m=
iddle.
> > > > > Without the patch, if you enable rfkill, suspend, resume, dis=
able
> > > > > rfkill, then your interface cannot be brought up.
> > > >=20
> > > > I guess you refer to the situation where the interface is up, r=
ight?
> > > > Something like:
> > > >=20
> > > > - ifconfig wlan0 up
> > > > - press killswitch (kill wireless)
> > > > - suspend
> > > > - resume
> > > > - press killswitch (enable wireless)
> > > > - here the interface should still be up
> > > >=20
> > > > As the interface is/was up, mac80211's resume handler should re=
start the
> > > > adapter and thus we wouldn't need to restart the adapter in the
> > > > rfkill-handler, or did I miss anything?
> > >=20
> > > Yes, the resume handler will start the adapter (call "start"), bu=
t the
> > > actions done by it will exit early because of rfkill being enable=
d. The
> > > STATUS_ALIVE bit will thus not be set after this is completed. La=
ter,
> > > when user disables rfkill, we want to restart the adapter to get =
all
> > > this corrected, but this call currently fails because of this che=
ck.
> >=20
> > Got it, thanks for the explanation.
> >=20
> > Nevertheless, removing the check will result in restarting the adap=
ter even
> > if the interface is down. So, I agree that we have a problem here b=
ut I do
> > not agree with the solution ;)
>=20
> I agree that it is not efficient ... but it seems harmless.

Assume the following situation:

You only have one killswitch for both, wireless and bluetooth. The wire=
less
interface is down because it is unused and the user wants to use blueto=
oth
and enables it via the killswitch which also means that wireless gets
unkilled. Now restarting the adapter needs more power then keeping the
adapter down. In short: if the interface is down the user (space) does =
not
want to use the interface and probably wants to save power as well.

> > Maybe taking the interface up (not only pseudo-up, as done currentl=
y) should
> > be allowed even if wireless is killed? We already allow the interfa=
ce to
> > stay up when the adapter gets rfkilled.
>=20
> Knowing that rfkill is enabled enables us to save power by not bringi=
ng
> everything up.

Yes, but user (space) knows that the wireless card consumes more power =
if the
interface is up. And if you use NM for example it will just take the in=
terface
down when the killswitch gets activated.

Another solution would be to
1) not allow the interface to go up when it is rfkilled, and
2) take the interface down when it gets rfkilled

However, I'm not sure if 2 is possible without help of mac80211 and if =
it
makes sense to change an interface state from within the driver.

Helmut

2009-03-27 16:25:29

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

Hi John,

On Fri, 2009-03-27 at 09:03 -0700, reinette chatre wrote:
> On Thu, 2009-03-26 at 23:18 -0700, Helmut Schaa wrote:
> > Hi again,
> >=20
> > Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > > Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit ge=
t
> > > clear in __iwl_down() function; the additional checking will fail=
and
> > > cause RF can not be turn back on.
> >=20
> > Maybe just checking for is_open instead of STATUS_ALIVE will fix th=
e issue
> > too? I only had a quick look but it looks possible. AFAICS is_open =
indicates
> > if the interface was taken up by mac80211. So if the interface is u=
p but
> > STATUS_ALIVE is not set we want to restart the adapter after unkill=
ing but
> > if the interface is still down (is_open=3D=3D0) we do not want to r=
estart the
> > adapter. Furthermore, is_open is also set in case the interface was=
taken
> > up but wireless was rfkilled.
>=20
> Very good idea ... I'm looking into this.

Could you please drop this patch? We will send a new fix for this
problem using Helmut's suggestion.

Thank you

Reinette

2009-03-26 17:08:43

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

From: Wey-Yi Guy <[email protected]>

Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit get
clear in __iwl_down() function; the additional checking will fail and
cause RF can not be turn back on.

Signed-off-by: Wey-Yi Guy <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-core.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index e1b5472..7560d6f 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -2056,8 +2056,7 @@ void iwl_bg_rf_kill(struct work_struct *work)
IWL_DEBUG_RF_KILL(priv,
"HW and/or SW RF Kill no longer active, restarting "
"device\n");
- if (!test_bit(STATUS_EXIT_PENDING, &priv->status) &&
- test_bit(STATUS_ALIVE, &priv->status))
+ if (!test_bit(STATUS_EXIT_PENDING, &priv->status))
queue_work(priv->workqueue, &priv->restart);
} else {
/* make sure mac80211 stop sending Tx frame */
--
1.5.6.3


2009-03-26 17:08:43

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 1/3] iwlwifi: merge and better support of suspend/resume for iwlagn and iwl3945

From: Wey-Yi Guy <[email protected]>

With mac80211's help to call stop() and start() in mac80211
suspend/resume function, both iwlagn and iwl3945 no longer calling
stop() and start(); remove un-necessary STATUS_IN_SUSPEND bit from both
header files and functions,

Move apm_ops.stop() function into pci_suspend() to ensure
DMA is stopped before go into suspend mode.

iwl3945 has the similar suspend/resume function as iwlagn, so move both
functions to iwlcore to be shared by both drivers.

Signed-off-by: Wey-Yi Guy <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-3945.h | 1 -
drivers/net/wireless/iwlwifi/iwl-agn.c | 54 ++---------------------
drivers/net/wireless/iwlwifi/iwl-core.c | 40 +++++++++++++++++
drivers/net/wireless/iwlwifi/iwl-core.h | 5 ++-
drivers/net/wireless/iwlwifi/iwl-debugfs.c | 2 -
drivers/net/wireless/iwlwifi/iwl3945-base.c | 63 +++-----------------------
6 files changed, 56 insertions(+), 109 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-3945.h b/drivers/net/wireless/iwlwifi/iwl-3945.h
index ab7aaf6..29bc0d2 100644
--- a/drivers/net/wireless/iwlwifi/iwl-3945.h
+++ b/drivers/net/wireless/iwlwifi/iwl-3945.h
@@ -162,7 +162,6 @@ struct iwl3945_frame {
#define STATUS_TEMPERATURE 8
#define STATUS_GEO_CONFIGURED 9
#define STATUS_EXIT_PENDING 10
-#define STATUS_IN_SUSPEND 11
#define STATUS_STATISTICS 12
#define STATUS_SCANNING 13
#define STATUS_SCAN_ABORTING 14
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 663dc83..669b435 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -1654,7 +1654,7 @@ static void __iwl_down(struct iwl_priv *priv)
ieee80211_stop_queues(priv->hw);

/* If we have not previously called iwl_init() then
- * clear all bits but the RF Kill and SUSPEND bits and return */
+ * clear all bits but the RF Kill bits and return */
if (!iwl_is_init(priv)) {
priv->status = test_bit(STATUS_RF_KILL_HW, &priv->status) <<
STATUS_RF_KILL_HW |
@@ -1662,23 +1662,19 @@ static void __iwl_down(struct iwl_priv *priv)
STATUS_RF_KILL_SW |
test_bit(STATUS_GEO_CONFIGURED, &priv->status) <<
STATUS_GEO_CONFIGURED |
- test_bit(STATUS_IN_SUSPEND, &priv->status) <<
- STATUS_IN_SUSPEND |
test_bit(STATUS_EXIT_PENDING, &priv->status) <<
STATUS_EXIT_PENDING;
goto exit;
}

- /* ...otherwise clear out all the status bits but the RF Kill and
- * SUSPEND bits and continue taking the NIC down. */
+ /* ...otherwise clear out all the status bits but the RF Kill
+ * bits and continue taking the NIC down. */
priv->status &= test_bit(STATUS_RF_KILL_HW, &priv->status) <<
STATUS_RF_KILL_HW |
test_bit(STATUS_RF_KILL_SW, &priv->status) <<
STATUS_RF_KILL_SW |
test_bit(STATUS_GEO_CONFIGURED, &priv->status) <<
STATUS_GEO_CONFIGURED |
- test_bit(STATUS_IN_SUSPEND, &priv->status) <<
- STATUS_IN_SUSPEND |
test_bit(STATUS_FW_ERROR, &priv->status) <<
STATUS_FW_ERROR |
test_bit(STATUS_EXIT_PENDING, &priv->status) <<
@@ -1703,7 +1699,7 @@ static void __iwl_down(struct iwl_priv *priv)
udelay(5);

/* FIXME: apm_ops.suspend(priv) */
- if (exit_pending || test_bit(STATUS_IN_SUSPEND, &priv->status))
+ if (exit_pending)
priv->cfg->ops->lib->apm_ops.stop(priv);
else
priv->cfg->ops->lib->apm_ops.reset(priv);
@@ -2064,9 +2060,6 @@ static int iwl_mac_start(struct ieee80211_hw *hw)

IWL_DEBUG_INFO(priv, "Start UP work done.\n");

- if (test_bit(STATUS_IN_SUSPEND, &priv->status))
- return 0;
-
/* Wait for START_ALIVE from Run Time ucode. Otherwise callbacks from
* mac80211 will not be run successfully. */
ret = wait_event_interruptible_timeout(priv->wait_command_queue,
@@ -3566,45 +3559,6 @@ static void __devexit iwl_pci_remove(struct pci_dev *pdev)
ieee80211_free_hw(priv->hw);
}

-#ifdef CONFIG_PM
-
-static int iwl_pci_suspend(struct pci_dev *pdev, pm_message_t state)
-{
- struct iwl_priv *priv = pci_get_drvdata(pdev);
-
- if (priv->is_open) {
- set_bit(STATUS_IN_SUSPEND, &priv->status);
- iwl_mac_stop(priv->hw);
- priv->is_open = 1;
- }
-
- pci_save_state(pdev);
- pci_disable_device(pdev);
- pci_set_power_state(pdev, PCI_D3hot);
-
- return 0;
-}
-
-static int iwl_pci_resume(struct pci_dev *pdev)
-{
- struct iwl_priv *priv = pci_get_drvdata(pdev);
- int ret;
-
- pci_set_power_state(pdev, PCI_D0);
- ret = pci_enable_device(pdev);
- if (ret)
- return ret;
- pci_restore_state(pdev);
- iwl_enable_interrupts(priv);
-
- if (priv->is_open)
- iwl_mac_start(priv->hw);
-
- clear_bit(STATUS_IN_SUSPEND, &priv->status);
- return 0;
-}
-
-#endif /* CONFIG_PM */

/*****************************************************************************
*
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 4b1298c..e1b5472 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -2115,3 +2115,43 @@ void iwl_rx_reply_error(struct iwl_priv *priv,
}
EXPORT_SYMBOL(iwl_rx_reply_error);

+#ifdef CONFIG_PM
+
+int iwl_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+ struct iwl_priv *priv = pci_get_drvdata(pdev);
+
+ /*
+ * This function is called when system goes into suspend state
+ * mac80211 will call iwl_mac_stop() from the mac80211 suspend function
+ * first but since iwl_mac_stop() has no knowledge of who the caller is,
+ * it will not call apm_ops.stop() to stop the DMA operation.
+ * Calling apm_ops.stop here to make sure we stop the DMA.
+ */
+ priv->cfg->ops->lib->apm_ops.stop(priv);
+
+ pci_save_state(pdev);
+ pci_disable_device(pdev);
+ pci_set_power_state(pdev, PCI_D3hot);
+
+ return 0;
+}
+EXPORT_SYMBOL(iwl_pci_suspend);
+
+int iwl_pci_resume(struct pci_dev *pdev)
+{
+ struct iwl_priv *priv = pci_get_drvdata(pdev);
+ int ret;
+
+ pci_set_power_state(pdev, PCI_D0);
+ ret = pci_enable_device(pdev);
+ if (ret)
+ return ret;
+ pci_restore_state(pdev);
+ iwl_enable_interrupts(priv);
+
+ return 0;
+}
+EXPORT_SYMBOL(iwl_pci_resume);
+
+#endif /* CONFIG_PM */
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.h b/drivers/net/wireless/iwlwifi/iwl-core.h
index a8eac8c..5d49045 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.h
+++ b/drivers/net/wireless/iwlwifi/iwl-core.h
@@ -432,6 +432,10 @@ static inline u16 iwl_pcie_link_ctl(struct iwl_priv *priv)
pci_read_config_word(priv->pci_dev, pos + PCI_EXP_LNKCTL, &pci_lnk_ctl);
return pci_lnk_ctl;
}
+#ifdef CONFIG_PM
+int iwl_pci_suspend(struct pci_dev *pdev, pm_message_t state);
+int iwl_pci_resume(struct pci_dev *pdev);
+#endif /* CONFIG_PM */

/*****************************************************
* Error Handling Debugging
@@ -458,7 +462,6 @@ void iwlcore_free_geos(struct iwl_priv *priv);
#define STATUS_TEMPERATURE 8
#define STATUS_GEO_CONFIGURED 9
#define STATUS_EXIT_PENDING 10
-#define STATUS_IN_SUSPEND 11
#define STATUS_STATISTICS 12
#define STATUS_SCANNING 13
#define STATUS_SCAN_ABORTING 14
diff --git a/drivers/net/wireless/iwlwifi/iwl-debugfs.c b/drivers/net/wireless/iwlwifi/iwl-debugfs.c
index 64eb585..af1d121 100644
--- a/drivers/net/wireless/iwlwifi/iwl-debugfs.c
+++ b/drivers/net/wireless/iwlwifi/iwl-debugfs.c
@@ -456,8 +456,6 @@ static ssize_t iwl_dbgfs_status_read(struct file *file,
test_bit(STATUS_GEO_CONFIGURED, &priv->status));
pos += scnprintf(buf + pos, bufsz - pos, "STATUS_EXIT_PENDING:\t %d\n",
test_bit(STATUS_EXIT_PENDING, &priv->status));
- pos += scnprintf(buf + pos, bufsz - pos, "STATUS_IN_SUSPEND:\t %d\n",
- test_bit(STATUS_IN_SUSPEND, &priv->status));
pos += scnprintf(buf + pos, bufsz - pos, "STATUS_STATISTICS:\t %d\n",
test_bit(STATUS_STATISTICS, &priv->status));
pos += scnprintf(buf + pos, bufsz - pos, "STATUS_SCANNING:\t %d\n",
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 279d10c..33f5b00 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -2996,7 +2996,7 @@ static void __iwl3945_down(struct iwl_priv *priv)
ieee80211_stop_queues(priv->hw);

/* If we have not previously called iwl3945_init() then
- * clear all bits but the RF Kill and SUSPEND bits and return */
+ * clear all bits but the RF Kill bits and return */
if (!iwl_is_init(priv)) {
priv->status = test_bit(STATUS_RF_KILL_HW, &priv->status) <<
STATUS_RF_KILL_HW |
@@ -3004,23 +3004,19 @@ static void __iwl3945_down(struct iwl_priv *priv)
STATUS_RF_KILL_SW |
test_bit(STATUS_GEO_CONFIGURED, &priv->status) <<
STATUS_GEO_CONFIGURED |
- test_bit(STATUS_IN_SUSPEND, &priv->status) <<
- STATUS_IN_SUSPEND |
test_bit(STATUS_EXIT_PENDING, &priv->status) <<
STATUS_EXIT_PENDING;
goto exit;
}

- /* ...otherwise clear out all the status bits but the RF Kill and
- * SUSPEND bits and continue taking the NIC down. */
+ /* ...otherwise clear out all the status bits but the RF Kill
+ * bits and continue taking the NIC down. */
priv->status &= test_bit(STATUS_RF_KILL_HW, &priv->status) <<
STATUS_RF_KILL_HW |
test_bit(STATUS_RF_KILL_SW, &priv->status) <<
STATUS_RF_KILL_SW |
test_bit(STATUS_GEO_CONFIGURED, &priv->status) <<
STATUS_GEO_CONFIGURED |
- test_bit(STATUS_IN_SUSPEND, &priv->status) <<
- STATUS_IN_SUSPEND |
test_bit(STATUS_FW_ERROR, &priv->status) <<
STATUS_FW_ERROR |
test_bit(STATUS_EXIT_PENDING, &priv->status) <<
@@ -3044,7 +3040,7 @@ static void __iwl3945_down(struct iwl_priv *priv)

udelay(5);

- if (exit_pending || test_bit(STATUS_IN_SUSPEND, &priv->status))
+ if (exit_pending)
priv->cfg->ops->lib->apm_ops.stop(priv);
else
priv->cfg->ops->lib->apm_ops.reset(priv);
@@ -3097,10 +3093,8 @@ static int __iwl3945_up(struct iwl_priv *priv)
clear_bit(STATUS_RF_KILL_HW, &priv->status);
else {
set_bit(STATUS_RF_KILL_HW, &priv->status);
- if (!test_bit(STATUS_IN_SUSPEND, &priv->status)) {
- IWL_WARN(priv, "Radio disabled by HW RF Kill switch\n");
- return -ENODEV;
- }
+ IWL_WARN(priv, "Radio disabled by HW RF Kill switch\n");
+ return -ENODEV;
}

iwl_write32(priv, CSR_INT, 0xFFFFFFFF);
@@ -3592,9 +3586,6 @@ static int iwl3945_mac_start(struct ieee80211_hw *hw)

IWL_DEBUG_INFO(priv, "Start UP work.\n");

- if (test_bit(STATUS_IN_SUSPEND, &priv->status))
- return 0;
-
/* Wait for START_ALIVE from ucode. Otherwise callbacks from
* mac80211 will not be run successfully. */
ret = wait_event_interruptible_timeout(priv->wait_command_queue,
@@ -5231,44 +5222,6 @@ static void __devexit iwl3945_pci_remove(struct pci_dev *pdev)
ieee80211_free_hw(priv->hw);
}

-#ifdef CONFIG_PM
-
-static int iwl3945_pci_suspend(struct pci_dev *pdev, pm_message_t state)
-{
- struct iwl_priv *priv = pci_get_drvdata(pdev);
-
- if (priv->is_open) {
- set_bit(STATUS_IN_SUSPEND, &priv->status);
- iwl3945_mac_stop(priv->hw);
- priv->is_open = 1;
- }
-
- pci_save_state(pdev);
- pci_disable_device(pdev);
- pci_set_power_state(pdev, PCI_D3hot);
-
- return 0;
-}
-
-static int iwl3945_pci_resume(struct pci_dev *pdev)
-{
- struct iwl_priv *priv = pci_get_drvdata(pdev);
- int ret;
-
- pci_set_power_state(pdev, PCI_D0);
- ret = pci_enable_device(pdev);
- if (ret)
- return ret;
- pci_restore_state(pdev);
-
- if (priv->is_open)
- iwl3945_mac_start(priv->hw);
-
- clear_bit(STATUS_IN_SUSPEND, &priv->status);
- return 0;
-}
-
-#endif /* CONFIG_PM */

/*****************************************************************************
*
@@ -5282,8 +5235,8 @@ static struct pci_driver iwl3945_driver = {
.probe = iwl3945_pci_probe,
.remove = __devexit_p(iwl3945_pci_remove),
#ifdef CONFIG_PM
- .suspend = iwl3945_pci_suspend,
- .resume = iwl3945_pci_resume,
+ .suspend = iwl_pci_suspend,
+ .resume = iwl_pci_resume,
#endif
};

--
1.5.6.3


2009-03-27 15:58:09

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

On Thu, 2009-03-26 at 23:18 -0700, Helmut Schaa wrote:
> Hi again,
>=20
> Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit get
> > clear in __iwl_down() function; the additional checking will fail a=
nd
> > cause RF can not be turn back on.
>=20
> Maybe just checking for is_open instead of STATUS_ALIVE will fix the =
issue
> too? I only had a quick look but it looks possible. AFAICS is_open in=
dicates
> if the interface was taken up by mac80211. So if the interface is up =
but
> STATUS_ALIVE is not set we want to restart the adapter after unkillin=
g but
> if the interface is still down (is_open=3D=3D0) we do not want to res=
tart the
> adapter. Furthermore, is_open is also set in case the interface was t=
aken
> up but wireless was rfkilled.

Very good idea ... I'm looking into this.

Reinette

2009-03-26 17:08:44

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 3/3] iwl3945: use iwl_mac_conf_tx

From: Abhijeet Kolekar <[email protected]>

3945 now uses iwl_mac_conf_tx.

Signed-off-by: Abhijeet Kolekar <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 43 ------------------------
drivers/net/wireless/iwlwifi/iwl-core.c | 43 ++++++++++++++++++++++++
drivers/net/wireless/iwlwifi/iwl-core.h | 2 +
drivers/net/wireless/iwlwifi/iwl3945-base.c | 48 +--------------------------
4 files changed, 46 insertions(+), 90 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 669b435..51f6a01 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -2628,49 +2628,6 @@ static int iwl_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
return ret;
}

-static int iwl_mac_conf_tx(struct ieee80211_hw *hw, u16 queue,
- const struct ieee80211_tx_queue_params *params)
-{
- struct iwl_priv *priv = hw->priv;
- unsigned long flags;
- int q;
-
- IWL_DEBUG_MAC80211(priv, "enter\n");
-
- if (!iwl_is_ready_rf(priv)) {
- IWL_DEBUG_MAC80211(priv, "leave - RF not ready\n");
- return -EIO;
- }
-
- if (queue >= AC_NUM) {
- IWL_DEBUG_MAC80211(priv, "leave - queue >= AC_NUM %d\n", queue);
- return 0;
- }
-
- q = AC_NUM - 1 - queue;
-
- spin_lock_irqsave(&priv->lock, flags);
-
- priv->qos_data.def_qos_parm.ac[q].cw_min = cpu_to_le16(params->cw_min);
- priv->qos_data.def_qos_parm.ac[q].cw_max = cpu_to_le16(params->cw_max);
- priv->qos_data.def_qos_parm.ac[q].aifsn = params->aifs;
- priv->qos_data.def_qos_parm.ac[q].edca_txop =
- cpu_to_le16((params->txop * 32));
-
- priv->qos_data.def_qos_parm.ac[q].reserved1 = 0;
- priv->qos_data.qos_active = 1;
-
- if (priv->iw_mode == NL80211_IFTYPE_AP)
- iwl_activate_qos(priv, 1);
- else if (priv->assoc_id && iwl_is_associated(priv))
- iwl_activate_qos(priv, 0);
-
- spin_unlock_irqrestore(&priv->lock, flags);
-
- IWL_DEBUG_MAC80211(priv, "leave\n");
- return 0;
-}
-
static int iwl_mac_ampdu_action(struct ieee80211_hw *hw,
enum ieee80211_ampdu_mlme_action action,
struct ieee80211_sta *sta, u16 tid, u16 *ssn)
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 7560d6f..02fa5c7 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -2114,6 +2114,49 @@ void iwl_rx_reply_error(struct iwl_priv *priv,
}
EXPORT_SYMBOL(iwl_rx_reply_error);

+int iwl_mac_conf_tx(struct ieee80211_hw *hw, u16 queue,
+ const struct ieee80211_tx_queue_params *params)
+{
+ struct iwl_priv *priv = hw->priv;
+ unsigned long flags;
+ int q;
+
+ IWL_DEBUG_MAC80211(priv, "enter\n");
+
+ if (!iwl_is_ready_rf(priv)) {
+ IWL_DEBUG_MAC80211(priv, "leave - RF not ready\n");
+ return -EIO;
+ }
+
+ if (queue >= AC_NUM) {
+ IWL_DEBUG_MAC80211(priv, "leave - queue >= AC_NUM %d\n", queue);
+ return 0;
+ }
+
+ q = AC_NUM - 1 - queue;
+
+ spin_lock_irqsave(&priv->lock, flags);
+
+ priv->qos_data.def_qos_parm.ac[q].cw_min = cpu_to_le16(params->cw_min);
+ priv->qos_data.def_qos_parm.ac[q].cw_max = cpu_to_le16(params->cw_max);
+ priv->qos_data.def_qos_parm.ac[q].aifsn = params->aifs;
+ priv->qos_data.def_qos_parm.ac[q].edca_txop =
+ cpu_to_le16((params->txop * 32));
+
+ priv->qos_data.def_qos_parm.ac[q].reserved1 = 0;
+ priv->qos_data.qos_active = 1;
+
+ if (priv->iw_mode == NL80211_IFTYPE_AP)
+ iwl_activate_qos(priv, 1);
+ else if (priv->assoc_id && iwl_is_associated(priv))
+ iwl_activate_qos(priv, 0);
+
+ spin_unlock_irqrestore(&priv->lock, flags);
+
+ IWL_DEBUG_MAC80211(priv, "leave\n");
+ return 0;
+}
+EXPORT_SYMBOL(iwl_mac_conf_tx);
#ifdef CONFIG_PM

int iwl_pci_suspend(struct pci_dev *pdev, pm_message_t state)
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.h b/drivers/net/wireless/iwlwifi/iwl-core.h
index 5d49045..d56edcd 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.h
+++ b/drivers/net/wireless/iwlwifi/iwl-core.h
@@ -225,6 +225,8 @@ struct ieee80211_hw *iwl_alloc_all(struct iwl_cfg *cfg,
void iwl_hw_detect(struct iwl_priv *priv);
void iwl_reset_qos(struct iwl_priv *priv);
void iwl_activate_qos(struct iwl_priv *priv, u8 force);
+int iwl_mac_conf_tx(struct ieee80211_hw *hw, u16 queue,
+ const struct ieee80211_tx_queue_params *params);
void iwl_set_rxon_hwcrypto(struct iwl_priv *priv, int hw_decrypt);
int iwl_check_rxon_cmd(struct iwl_priv *priv);
int iwl_full_rxon_required(struct iwl_priv *priv);
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 33f5b00..c29189b 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -4100,52 +4100,6 @@ static int iwl3945_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
return ret;
}

-static int iwl3945_mac_conf_tx(struct ieee80211_hw *hw, u16 queue,
- const struct ieee80211_tx_queue_params *params)
-{
- struct iwl_priv *priv = hw->priv;
- unsigned long flags;
- int q;
-
- IWL_DEBUG_MAC80211(priv, "enter\n");
-
- if (!iwl_is_ready_rf(priv)) {
- IWL_DEBUG_MAC80211(priv, "leave - RF not ready\n");
- return -EIO;
- }
-
- if (queue >= AC_NUM) {
- IWL_DEBUG_MAC80211(priv, "leave - queue >= AC_NUM %d\n", queue);
- return 0;
- }
-
- q = AC_NUM - 1 - queue;
-
- spin_lock_irqsave(&priv->lock, flags);
-
- priv->qos_data.def_qos_parm.ac[q].cw_min = cpu_to_le16(params->cw_min);
- priv->qos_data.def_qos_parm.ac[q].cw_max = cpu_to_le16(params->cw_max);
- priv->qos_data.def_qos_parm.ac[q].aifsn = params->aifs;
- priv->qos_data.def_qos_parm.ac[q].edca_txop =
- cpu_to_le16((params->txop * 32));
-
- priv->qos_data.def_qos_parm.ac[q].reserved1 = 0;
- priv->qos_data.qos_active = 1;
-
- spin_unlock_irqrestore(&priv->lock, flags);
-
- mutex_lock(&priv->mutex);
- if (priv->iw_mode == NL80211_IFTYPE_AP)
- iwl_activate_qos(priv, 1);
- else if (priv->assoc_id && iwl_is_associated(priv))
- iwl_activate_qos(priv, 0);
-
- mutex_unlock(&priv->mutex);
-
- IWL_DEBUG_MAC80211(priv, "leave\n");
- return 0;
-}
-
static int iwl3945_mac_get_tx_stats(struct ieee80211_hw *hw,
struct ieee80211_tx_queue_stats *stats)
{
@@ -4809,7 +4763,7 @@ static struct ieee80211_ops iwl3945_hw_ops = {
.configure_filter = iwl_configure_filter,
.set_key = iwl3945_mac_set_key,
.get_tx_stats = iwl3945_mac_get_tx_stats,
- .conf_tx = iwl3945_mac_conf_tx,
+ .conf_tx = iwl_mac_conf_tx,
.reset_tsf = iwl3945_mac_reset_tsf,
.bss_info_changed = iwl3945_bss_info_changed,
.hw_scan = iwl_mac_hw_scan
--
1.5.6.3


2009-03-26 18:17:39

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2/3] iwlwifi: remove STATUS_ALIVE checking from rf_kill

On Thu, 2009-03-26 at 10:50 -0700, Helmut Schaa wrote:
> Hi,
>=20
> Am Donnerstag, 26. M=C3=A4rz 2009 schrieb Reinette Chatre:
> > From: Wey-Yi Guy <[email protected]>
> >=20
> > Remove STATUS_ALIVE checking when HW RF KILL disabled, the bit get
> > clear in __iwl_down() function; the additional checking will fail a=
nd
> > cause RF can not be turn back on.
>=20
> Are you sure this is needed? I'd argue we should only restart the ada=
pter
> if it was alive when it got rf_killed. In case the adapter was rf_kil=
led
> while the interface was down I don't think we want to restart the ada=
pter
> immediately but first when the interface is taken up again.

We also need to consider if a suspend/resume happens in the middle.
Without the patch, if you enable rfkill, suspend, resume, disable
rfkill, then your interface cannot be brought up.

Reinette