[PATCH 2.6.30] iwl3945: fix rfkill switch

[PATCH 2.6.30] iwl3945: fix rfkill switch

[Stanislaw Gruszka]

Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
we have race conditions which make unable turn wifi radio on, after enable
and disable again killswitch. I can observe this problem on my laptop
with iwl3945 device.

In rfkill core HW switch and SW switch are separate 'states'. Device can
be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
turned on, only if both bits are cleared.

In this particular race conditions, radio can not be turned on if in driver
STATUS_RF_KILL_SW bit is set, and rfkill core is in state
RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
rfkill->toggle_radio(). This situation can be entered in case:

- killswitch is turned on
- rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
  also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
- iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
  force rfkill to move to RFKILL_STATE_HARD_BLOCKED
- killsiwtch is turend off
- driver clear STATUS_RF_KILL_HW
- rfkill core is unable to clear STATUS_RF_KILL_SW in driver

Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
cause move to the same situation.

In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
subsystem SW switch bit before HW switch bit to move rfkill subsystem
to SOFT_BLOCK rather than HARD_BLOCK.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
I'm not sure if this is good candidate for stable as this is not backport
of upstream commit. Also I did not test this patch with other iwlwifi devices,
only with iwl3945.

 drivers/net/wireless/iwlwifi/iwl-rfkill.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
index 2ad9faf..d6b6098 100644
--- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
+++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
@@ -54,21 +54,28 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
 	case RFKILL_STATE_UNBLOCKED:
 		if (iwl_is_rfkill_hw(priv)) {
 			err = -EBUSY;
-			goto out_unlock;
+			/* pass error to rfkill core to make it state HARD
+			 * BLOCKED and disable software kill switch */
 		}
 		iwl_radio_kill_sw_enable_radio(priv);
 		break;
 	case RFKILL_STATE_SOFT_BLOCKED:
 		iwl_radio_kill_sw_disable_radio(priv);
+		/* rfkill->mutex lock is taken */
+		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
+			/* force rfkill core state to be SOFT BLOCKED,
+			 * otherwise core will be unable to disable software
+			 * kill switch */
+			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
+		}
 		break;
 	default:
 		IWL_WARN(priv, "we received unexpected RFKILL state %d\n",
 			state);
 		break;
 	}
-out_unlock:
-	mutex_unlock(&priv->mutex);
 
+	mutex_unlock(&priv->mutex);
 	return err;
 }
 
@@ -132,14 +139,11 @@ void iwl_rfkill_set_hw_state(struct iwl_priv *priv)
 	if (!priv->rfkill)
 		return;
 
-	if (iwl_is_rfkill_hw(priv)) {
+	if (iwl_is_rfkill_sw(priv))
+		rfkill_force_state(priv->rfkill, RFKILL_STATE_SOFT_BLOCKED);
+	else if (iwl_is_rfkill_hw(priv))
 		rfkill_force_state(priv->rfkill, RFKILL_STATE_HARD_BLOCKED);
-		return;
-	}
-
-	if (!iwl_is_rfkill_sw(priv))
-		rfkill_force_state(priv->rfkill, RFKILL_STATE_UNBLOCKED);
 	else
-		rfkill_force_state(priv->rfkill, RFKILL_STATE_SOFT_BLOCKED);
+		rfkill_force_state(priv->rfkill, RFKILL_STATE_UNBLOCKED);
 }
 EXPORT_SYMBOL(iwl_rfkill_set_hw_state);
-- 
1.6.2.5

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[John W. Linville]

On Tue, Aug 04, 2009 at 02:35:50PM +0200, Stanislaw Gruszka wrote:
> Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> we have race conditions which make unable turn wifi radio on, after enable
> and disable again killswitch. I can observe this problem on my laptop
> with iwl3945 device.
> 
> In rfkill core HW switch and SW switch are separate 'states'. Device can
> be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> turned on, only if both bits are cleared.
> 
> In this particular race conditions, radio can not be turned on if in driver
> STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> rfkill->toggle_radio(). This situation can be entered in case:
> 
> - killswitch is turned on
> - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
>   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
>   force rfkill to move to RFKILL_STATE_HARD_BLOCKED
> - killsiwtch is turend off
> - driver clear STATUS_RF_KILL_HW
> - rfkill core is unable to clear STATUS_RF_KILL_SW in driver
> 
> Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> cause move to the same situation.
> 
> In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> subsystem SW switch bit before HW switch bit to move rfkill subsystem
> to SOFT_BLOCK rather than HARD_BLOCK.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> I'm not sure if this is good candidate for stable as this is not backport
> of upstream commit. Also I did not test this patch with other iwlwifi devices,
> only with iwl3945.

stable is about the only place it can go, since it fixes a problem
that doesn't exist in 2.6.31...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.
			¡Viva Honduras Libre!

Re: [stable] [PATCH 2.6.30] iwl3945: fix rfkill switch

[Greg KH]

On Tue, Aug 04, 2009 at 08:49:51AM -0400, John W. Linville wrote:
> On Tue, Aug 04, 2009 at 02:35:50PM +0200, Stanislaw Gruszka wrote:
> > Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> > we have race conditions which make unable turn wifi radio on, after enable
> > and disable again killswitch. I can observe this problem on my laptop
> > with iwl3945 device.
> > 
> > In rfkill core HW switch and SW switch are separate 'states'. Device can
> > be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> > RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> > STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> > turned on, only if both bits are cleared.
> > 
> > In this particular race conditions, radio can not be turned on if in driver
> > STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> > RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> > rfkill->toggle_radio(). This situation can be entered in case:
> > 
> > - killswitch is turned on
> > - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
> >   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> > - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
> >   force rfkill to move to RFKILL_STATE_HARD_BLOCKED
> > - killsiwtch is turend off
> > - driver clear STATUS_RF_KILL_HW
> > - rfkill core is unable to clear STATUS_RF_KILL_SW in driver
> > 
> > Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> > cause move to the same situation.
> > 
> > In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> > This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> > STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> > subsystem SW switch bit before HW switch bit to move rfkill subsystem
> > to SOFT_BLOCK rather than HARD_BLOCK.
> > 
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> > I'm not sure if this is good candidate for stable as this is not backport
> > of upstream commit. Also I did not test this patch with other iwlwifi devices,
> > only with iwl3945.
> 
> stable is about the only place it can go, since it fixes a problem
> that doesn't exist in 2.6.31...

Do you ack this for inclusion in 2.6.30-stable?

thanks,

greg k-h

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[reinette chatre]

Hi Stanislaw,

On Tue, 2009-08-04 at 05:35 -0700, Stanislaw Gruszka wrote:
> Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> we have race conditions which make unable turn wifi radio on, after enable
> and disable again killswitch. I can observe this problem on my laptop
> with iwl3945 device.
> 
> In rfkill core HW switch and SW switch are separate 'states'. Device can
> be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> turned on, only if both bits are cleared.
> 
> In this particular race conditions, radio can not be turned on if in driver
> STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> rfkill->toggle_radio(). This situation can be entered in case:
> 

I am trying to understand this race condition ...

> - killswitch is turned on
> - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
>   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
>   force rfkill to move to RFKILL_STATE_HARD_BLOCKED

ok - so at this point we have rfkill == RFKILL_STATE_HARD_BLOCKED, and
driver == STATE_RF_KILL_SW | STATE_RF_KILL_HW

> - killsiwtch is turend off
> - driver clear STATUS_RF_KILL_HW

at this point the driver should clear STATE_RF_KILL_HW and then call
iwl_rfkill_set_hw_state(). From what I can tell, in
iwl_rfkill_set_hw_state() the test for iwl_is_rfkill_sw() will cause the
driver to call rfkill_force_state for RFKILL_STATE_SOFT_BLOCKED

So, from what I understand after the above the status will be

rfkill == RFKILL_STATE_SOFT_BLOCKED, and driver == STATE_RF_KILL_SW 

> - rfkill core is unable to clear STATUS_RF_KILL_SW in driver

I do not understand why this is a problem here. Could you please
highlight what I am missing?

> 
> Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> cause move to the same situation.
> 
> In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> subsystem SW switch bit before HW switch bit to move rfkill subsystem
> to SOFT_BLOCK rather than HARD_BLOCK.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> I'm not sure if this is good candidate for stable as this is not backport
> of upstream commit. Also I did not test this patch with other iwlwifi devices,
> only with iwl3945.
> 
>  drivers/net/wireless/iwlwifi/iwl-rfkill.c |   24 ++++++++++++++----------
>  1 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> index 2ad9faf..d6b6098 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> @@ -54,21 +54,28 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
>  	case RFKILL_STATE_UNBLOCKED:
>  		if (iwl_is_rfkill_hw(priv)) {
>  			err = -EBUSY;
> -			goto out_unlock;
> +			/* pass error to rfkill core to make it state HARD
> +			 * BLOCKED and disable software kill switch */
>  		}
>  		iwl_radio_kill_sw_enable_radio(priv);
>  		break;
>  	case RFKILL_STATE_SOFT_BLOCKED:
>  		iwl_radio_kill_sw_disable_radio(priv);
> +		/* rfkill->mutex lock is taken */
> +		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> +			/* force rfkill core state to be SOFT BLOCKED,
> +			 * otherwise core will be unable to disable software
> +			 * kill switch */
> +			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> +		}

I understand that you are directly changing the rfkill internals because
the mutex is taken ... but this really does not seem right to directly
modify the rfkill state in this way.

>  		break;
>  	default:
>  		IWL_WARN(priv, "we received unexpected RFKILL state %d\n",
>  			state);
>  		break;
>  	}
> -out_unlock:
> -	mutex_unlock(&priv->mutex);
>  
> +	mutex_unlock(&priv->mutex);
>  	return err;
>  }
>  
> @@ -132,14 +139,11 @@ void iwl_rfkill_set_hw_state(struct iwl_priv *priv)
>  	if (!priv->rfkill)
>  		return;
>  
> -	if (iwl_is_rfkill_hw(priv)) {
> +	if (iwl_is_rfkill_sw(priv))
> +		rfkill_force_state(priv->rfkill, RFKILL_STATE_SOFT_BLOCKED);
> +	else if (iwl_is_rfkill_hw(priv))
>  		rfkill_force_state(priv->rfkill, RFKILL_STATE_HARD_BLOCKED);
> -		return;
> -	}
> -
> -	if (!iwl_is_rfkill_sw(priv))
> -		rfkill_force_state(priv->rfkill, RFKILL_STATE_UNBLOCKED);
>  	else
> -		rfkill_force_state(priv->rfkill, RFKILL_STATE_SOFT_BLOCKED);
> +		rfkill_force_state(priv->rfkill, RFKILL_STATE_UNBLOCKED);
>  }
>  EXPORT_SYMBOL(iwl_rfkill_set_hw_state);


Reinette

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[Stanislaw Gruszka]

On Wed, Aug 05, 2009 at 03:51:49PM -0700, reinette chatre wrote:
> Hi Stanislaw,
> 
> On Tue, 2009-08-04 at 05:35 -0700, Stanislaw Gruszka wrote:
> > Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> > we have race conditions which make unable turn wifi radio on, after enable
> > and disable again killswitch. I can observe this problem on my laptop
> > with iwl3945 device.
> > 
> > In rfkill core HW switch and SW switch are separate 'states'. Device can
> > be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> > RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> > STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> > turned on, only if both bits are cleared.
> > 
> > In this particular race conditions, radio can not be turned on if in driver
> > STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> > RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> > rfkill->toggle_radio(). This situation can be entered in case:
> > 
> 
> I am trying to understand this race condition ...
> 
> > - killswitch is turned on
> > - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
> >   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> > - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
> >   force rfkill to move to RFKILL_STATE_HARD_BLOCKED
> 
> ok - so at this point we have rfkill == RFKILL_STATE_HARD_BLOCKED, and
> driver == STATE_RF_KILL_SW | STATE_RF_KILL_HW
> 
> > - killsiwtch is turend off

Here rfkill core routines are called. Rfkill wants to clear STATUS_RF_KILL_SW
but it can not as state is RFKILL_STATE_HARD_BLOCKED.

> > - driver clear STATUS_RF_KILL_HW
> 
> at this point the driver should clear STATE_RF_KILL_HW and then call
> iwl_rfkill_set_hw_state(). From what I can tell, in
> iwl_rfkill_set_hw_state() the test for iwl_is_rfkill_sw() will cause the
> driver to call rfkill_force_state for RFKILL_STATE_SOFT_BLOCKED
> 
> So, from what I understand after the above the status will be
> 
> rfkill == RFKILL_STATE_SOFT_BLOCKED, and driver == STATE_RF_KILL_SW 

Thats right. But rfkill core no longer wants to manipulate state via
->toggle_radio() and radio stays disabled.
 
> > - rfkill core is unable to clear STATUS_RF_KILL_SW in driver
> 
> I do not understand why this is a problem here. Could you please
> highlight what I am missing?

In my description I miss the most important part, sorry. Race is when the
switches are performed in that order:

Radio enabled 
- rfkill SW on
- driver HW on
Radio disabled - ok
- rfkill SW off <- problem not clearing STATUS_RF_KILL_SW
- driver HW off
Radio disabled - wrong 

Everything is fine when actions are in that order:

Radio enabled
- rfkill SW on
- driver HW on
Radio disabled - ok 
- driver HW off
- rfkill SW off
Radio enabled - ok 

> > 
> > Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> > cause move to the same situation.
> > 
> > In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> > This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> > STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> > subsystem SW switch bit before HW switch bit to move rfkill subsystem
> > to SOFT_BLOCK rather than HARD_BLOCK.
> > 
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> > I'm not sure if this is good candidate for stable as this is not backport
> > of upstream commit. Also I did not test this patch with other iwlwifi devices,
> > only with iwl3945.
> > 
> >  drivers/net/wireless/iwlwifi/iwl-rfkill.c |   24 ++++++++++++++----------
> >  1 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > index 2ad9faf..d6b6098 100644
> > --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > @@ -54,21 +54,28 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> >  	case RFKILL_STATE_UNBLOCKED:
> >  		if (iwl_is_rfkill_hw(priv)) {
> >  			err = -EBUSY;
> > -			goto out_unlock;
> > +			/* pass error to rfkill core to make it state HARD
> > +			 * BLOCKED and disable software kill switch */
> >  		}
> >  		iwl_radio_kill_sw_enable_radio(priv);
> >  		break;
> >  	case RFKILL_STATE_SOFT_BLOCKED:
> >  		iwl_radio_kill_sw_disable_radio(priv);
> > +		/* rfkill->mutex lock is taken */
> > +		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> > +			/* force rfkill core state to be SOFT BLOCKED,
> > +			 * otherwise core will be unable to disable software
> > +			 * kill switch */
> > +			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> > +		}
> 
> I understand that you are directly changing the rfkill internals because
> the mutex is taken ... but this really does not seem right to directly
> modify the rfkill state in this way.

Agree this is dirty hack, but I did not find a better way. Eventually,
if we add call to rfkill_uevent(), this would behave the same
as rfkill_force_state() .

Cheers
Stanislaw

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[reinette chatre]

Hi Stanislaw,

On Thu, 2009-08-06 at 00:19 -0700, Stanislaw Gruszka wrote:
> On Wed, Aug 05, 2009 at 03:51:49PM -0700, reinette chatre wrote:
> > On Tue, 2009-08-04 at 05:35 -0700, Stanislaw Gruszka wrote:
> > > Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> > > we have race conditions which make unable turn wifi radio on, after enable
> > > and disable again killswitch. I can observe this problem on my laptop
> > > with iwl3945 device.
> > > 
> > > In rfkill core HW switch and SW switch are separate 'states'. Device can
> > > be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> > > RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> > > STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> > > turned on, only if both bits are cleared.
> > > 
> > > In this particular race conditions, radio can not be turned on if in driver
> > > STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> > > RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> > > rfkill->toggle_radio(). This situation can be entered in case:
> > > 
> > 
> > I am trying to understand this race condition ...
> > 
> > > - killswitch is turned on
> > > - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
> > >   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> > > - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
> > >   force rfkill to move to RFKILL_STATE_HARD_BLOCKED
> > 
> > ok - so at this point we have rfkill == RFKILL_STATE_HARD_BLOCKED, and
> > driver == STATE_RF_KILL_SW | STATE_RF_KILL_HW
> > 
> > > - killsiwtch is turend off
> 
> Here rfkill core routines are called. Rfkill wants to clear STATUS_RF_KILL_SW
> but it can not as state is RFKILL_STATE_HARD_BLOCKED.
> 
> > > - driver clear STATUS_RF_KILL_HW
> > 
> > at this point the driver should clear STATE_RF_KILL_HW and then call
> > iwl_rfkill_set_hw_state(). From what I can tell, in
> > iwl_rfkill_set_hw_state() the test for iwl_is_rfkill_sw() will cause the
> > driver to call rfkill_force_state for RFKILL_STATE_SOFT_BLOCKED
> > 
> > So, from what I understand after the above the status will be
> > 
> > rfkill == RFKILL_STATE_SOFT_BLOCKED, and driver == STATE_RF_KILL_SW 
> 
> Thats right. But rfkill core no longer wants to manipulate state via
> ->toggle_radio() and radio stays disabled.
>  
> > > - rfkill core is unable to clear STATUS_RF_KILL_SW in driver
> > 
> > I do not understand why this is a problem here. Could you please
> > highlight what I am missing?
> 
> In my description I miss the most important part, sorry. Race is when the
> switches are performed in that order:
> 
> Radio enabled 
> - rfkill SW on
> - driver HW on
> Radio disabled - ok
> - rfkill SW off <- problem not clearing STATUS_RF_KILL_SW
> - driver HW off
> Radio disabled - wrong 
> 
> Everything is fine when actions are in that order:
> 
> Radio enabled
> - rfkill SW on
> - driver HW on
> Radio disabled - ok 
> - driver HW off
> - rfkill SW off
> Radio enabled - ok 


Thanks for the explanation.

> 
> > > 
> > > Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> > > cause move to the same situation.
> > > 
> > > In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> > > This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> > > STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> > > subsystem SW switch bit before HW switch bit to move rfkill subsystem
> > > to SOFT_BLOCK rather than HARD_BLOCK.
> > > 
> > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > > ---
> > > I'm not sure if this is good candidate for stable as this is not backport
> > > of upstream commit. Also I did not test this patch with other iwlwifi devices,
> > > only with iwl3945.
> > > 
> > >  drivers/net/wireless/iwlwifi/iwl-rfkill.c |   24 ++++++++++++++----------
> > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > index 2ad9faf..d6b6098 100644
> > > --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > @@ -54,21 +54,28 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> > >  	case RFKILL_STATE_UNBLOCKED:
> > >  		if (iwl_is_rfkill_hw(priv)) {
> > >  			err = -EBUSY;
> > > -			goto out_unlock;
> > > +			/* pass error to rfkill core to make it state HARD
> > > +			 * BLOCKED and disable software kill switch */
> > >  		}
> > >  		iwl_radio_kill_sw_enable_radio(priv);
> > >  		break;
> > >  	case RFKILL_STATE_SOFT_BLOCKED:
> > >  		iwl_radio_kill_sw_disable_radio(priv);
> > > +		/* rfkill->mutex lock is taken */
> > > +		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> > > +			/* force rfkill core state to be SOFT BLOCKED,
> > > +			 * otherwise core will be unable to disable software
> > > +			 * kill switch */
> > > +			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> > > +		}
> > 
> > I understand that you are directly changing the rfkill internals because
> > the mutex is taken ... but this really does not seem right to directly
> > modify the rfkill state in this way.
> 
> Agree this is dirty hack, but I did not find a better way. Eventually,
> if we add call to rfkill_uevent(), this would behave the same
> as rfkill_force_state() .

Sorry, but I really do not understand why this code is needed. From what
you say rfkill can be in one of three states: RFKILL_STATE_UNBLOCKED,
RFKILL_STATE_SOFT_BLOCKED, or RFKILL_STATE_HARD_BLOCKED. From what I
understand the above code is called when there is an rfkill state change
and the new state is provided. So, only _one_ of the three states will
be provided as parameter. This state is then tested - so in the case
that you modified here the state has already been tested to be
RFKILL_STATE_SOFT_BLOCKED. How is it thus possible that it can be
RFKILL_STATE_HARD_BLOCKED also?

Reinette

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[Stanislaw Gruszka]

On Thu, Aug 06, 2009 at 01:15:58PM -0700, reinette chatre wrote:
> Hi Stanislaw,
> 
> On Thu, 2009-08-06 at 00:19 -0700, Stanislaw Gruszka wrote:
> > On Wed, Aug 05, 2009 at 03:51:49PM -0700, reinette chatre wrote:
> > > On Tue, 2009-08-04 at 05:35 -0700, Stanislaw Gruszka wrote:
> > > > Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> > > > we have race conditions which make unable turn wifi radio on, after enable
> > > > and disable again killswitch. I can observe this problem on my laptop
> > > > with iwl3945 device.
> > > > 
> > > > In rfkill core HW switch and SW switch are separate 'states'. Device can
> > > > be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> > > > RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> > > > STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> > > > turned on, only if both bits are cleared.
> > > > 
> > > > In this particular race conditions, radio can not be turned on if in driver
> > > > STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> > > > RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> > > > rfkill->toggle_radio(). This situation can be entered in case:
> > > > 
> > > 
> > > I am trying to understand this race condition ...
> > > 
> > > > - killswitch is turned on
> > > > - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
> > > >   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> > > > - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
> > > >   force rfkill to move to RFKILL_STATE_HARD_BLOCKED
> > > 
> > > ok - so at this point we have rfkill == RFKILL_STATE_HARD_BLOCKED, and
> > > driver == STATE_RF_KILL_SW | STATE_RF_KILL_HW
> > > 
> > > > - killsiwtch is turend off
> > 
> > Here rfkill core routines are called. Rfkill wants to clear STATUS_RF_KILL_SW
> > but it can not as state is RFKILL_STATE_HARD_BLOCKED.
> > 
> > > > - driver clear STATUS_RF_KILL_HW
> > > 
> > > at this point the driver should clear STATE_RF_KILL_HW and then call
> > > iwl_rfkill_set_hw_state(). From what I can tell, in
> > > iwl_rfkill_set_hw_state() the test for iwl_is_rfkill_sw() will cause the
> > > driver to call rfkill_force_state for RFKILL_STATE_SOFT_BLOCKED
> > > 
> > > So, from what I understand after the above the status will be
> > > 
> > > rfkill == RFKILL_STATE_SOFT_BLOCKED, and driver == STATE_RF_KILL_SW 
> > 
> > Thats right. But rfkill core no longer wants to manipulate state via
> > ->toggle_radio() and radio stays disabled.
> >  
> > > > - rfkill core is unable to clear STATUS_RF_KILL_SW in driver
> > > 
> > > I do not understand why this is a problem here. Could you please
> > > highlight what I am missing?
> > 
> > In my description I miss the most important part, sorry. Race is when the
> > switches are performed in that order:
> > 
> > Radio enabled 
> > - rfkill SW on
> > - driver HW on
> > Radio disabled - ok
> > - rfkill SW off <- problem not clearing STATUS_RF_KILL_SW
> > - driver HW off
> > Radio disabled - wrong 
> > 
> > Everything is fine when actions are in that order:
> > 
> > Radio enabled
> > - rfkill SW on
> > - driver HW on
> > Radio disabled - ok 
> > - driver HW off
> > - rfkill SW off
> > Radio enabled - ok 
> 
> 
> Thanks for the explanation.
> 
> > 
> > > > 
> > > > Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> > > > cause move to the same situation.
> > > > 
> > > > In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> > > > This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> > > > STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> > > > subsystem SW switch bit before HW switch bit to move rfkill subsystem
> > > > to SOFT_BLOCK rather than HARD_BLOCK.
> > > > 
> > > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > > > ---
> > > > I'm not sure if this is good candidate for stable as this is not backport
> > > > of upstream commit. Also I did not test this patch with other iwlwifi devices,
> > > > only with iwl3945.
> > > > 
> > > >  drivers/net/wireless/iwlwifi/iwl-rfkill.c |   24 ++++++++++++++----------
> > > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > index 2ad9faf..d6b6098 100644
> > > > --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > @@ -54,21 +54,28 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> > > >  	case RFKILL_STATE_UNBLOCKED:
> > > >  		if (iwl_is_rfkill_hw(priv)) {
> > > >  			err = -EBUSY;
> > > > -			goto out_unlock;
> > > > +			/* pass error to rfkill core to make it state HARD
> > > > +			 * BLOCKED and disable software kill switch */
> > > >  		}
> > > >  		iwl_radio_kill_sw_enable_radio(priv);
> > > >  		break;
> > > >  	case RFKILL_STATE_SOFT_BLOCKED:
> > > >  		iwl_radio_kill_sw_disable_radio(priv);
> > > > +		/* rfkill->mutex lock is taken */
> > > > +		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> > > > +			/* force rfkill core state to be SOFT BLOCKED,
> > > > +			 * otherwise core will be unable to disable software
> > > > +			 * kill switch */
> > > > +			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> > > > +		}
> > > 
> > > I understand that you are directly changing the rfkill internals because
> > > the mutex is taken ... but this really does not seem right to directly
> > > modify the rfkill state in this way.
> > 
> > Agree this is dirty hack, but I did not find a better way. Eventually,
> > if we add call to rfkill_uevent(), this would behave the same
> > as rfkill_force_state() .
> 
> Sorry, but I really do not understand why this code is needed. From what
> you say rfkill can be in one of three states: RFKILL_STATE_UNBLOCKED,
> RFKILL_STATE_SOFT_BLOCKED, or RFKILL_STATE_HARD_BLOCKED. From what I
> understand the above code is called when there is an rfkill state change
> and the new state is provided. So, only _one_ of the three states will
> be provided as parameter. This state is then tested - so in the case
> that you modified here the state has already been tested to be
> RFKILL_STATE_SOFT_BLOCKED. How is it thus possible that it can be
> RFKILL_STATE_HARD_BLOCKED also?

Local variable state != priv->rfkill->state . See rfkill_toggle_radio()
especially this part:

	if (force || state != rfkill->state) {
		retval = rfkill->toggle_radio(rfkill->data, state);
		/* never allow a HARD->SOFT downgrade! */
		if (!retval && rfkill->state != RFKILL_STATE_HARD_BLOCKED)
			rfkill->state = state;
	}

Without the change rfkill core will be in state RFKILL_STATE_HARD_BLOCKED and
latter will not clear STATE_RF_KILL_SW.
 
All hunks from the patch are needed on my laptop (lenoveo T60) to make
killswitch works as expected. Applying only some hunks from the patch helps
is one case or other, but without all hunks there is still possible to have
radio disabled when killswitch is off.

Regards
Stanislaw 

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[reinette chatre]

Hi Stanislaw,

On Thu, 2009-08-06 at 23:31 -0700, Stanislaw Gruszka wrote:
> On Thu, Aug 06, 2009 at 01:15:58PM -0700, reinette chatre wrote:
> > On Thu, 2009-08-06 at 00:19 -0700, Stanislaw Gruszka wrote:
> > > On Wed, Aug 05, 2009 at 03:51:49PM -0700, reinette chatre wrote:
> > > > On Tue, 2009-08-04 at 05:35 -0700, Stanislaw Gruszka wrote:
> > > > > Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> > > > > we have race conditions which make unable turn wifi radio on, after enable
> > > > > and disable again killswitch. I can observe this problem on my laptop
> > > > > with iwl3945 device.
> > > > > 
> > > > > In rfkill core HW switch and SW switch are separate 'states'. Device can
> > > > > be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> > > > > RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> > > > > STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> > > > > turned on, only if both bits are cleared.
> > > > > 
> > > > > In this particular race conditions, radio can not be turned on if in driver
> > > > > STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> > > > > RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> > > > > rfkill->toggle_radio(). This situation can be entered in case:
> > > > > 
> > > > 
> > > > I am trying to understand this race condition ...
> > > > 
> > > > > - killswitch is turned on
> > > > > - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
> > > > >   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> > > > > - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
> > > > >   force rfkill to move to RFKILL_STATE_HARD_BLOCKED
> > > > 
> > > > ok - so at this point we have rfkill == RFKILL_STATE_HARD_BLOCKED, and
> > > > driver == STATE_RF_KILL_SW | STATE_RF_KILL_HW
> > > > 
> > > > > - killsiwtch is turend off
> > > 
> > > Here rfkill core routines are called. Rfkill wants to clear STATUS_RF_KILL_SW
> > > but it can not as state is RFKILL_STATE_HARD_BLOCKED.
> > > 
> > > > > - driver clear STATUS_RF_KILL_HW
> > > > 
> > > > at this point the driver should clear STATE_RF_KILL_HW and then call
> > > > iwl_rfkill_set_hw_state(). From what I can tell, in
> > > > iwl_rfkill_set_hw_state() the test for iwl_is_rfkill_sw() will cause the
> > > > driver to call rfkill_force_state for RFKILL_STATE_SOFT_BLOCKED
> > > > 
> > > > So, from what I understand after the above the status will be
> > > > 
> > > > rfkill == RFKILL_STATE_SOFT_BLOCKED, and driver == STATE_RF_KILL_SW 
> > > 
> > > Thats right. But rfkill core no longer wants to manipulate state via
> > > ->toggle_radio() and radio stays disabled.
> > >  
> > > > > - rfkill core is unable to clear STATUS_RF_KILL_SW in driver
> > > > 
> > > > I do not understand why this is a problem here. Could you please
> > > > highlight what I am missing?
> > > 
> > > In my description I miss the most important part, sorry. Race is when the
> > > switches are performed in that order:
> > > 
> > > Radio enabled 
> > > - rfkill SW on
> > > - driver HW on
> > > Radio disabled - ok
> > > - rfkill SW off <- problem not clearing STATUS_RF_KILL_SW

Yes. I assume that what happens here is that rfkill notifies user that
state changes to RFKILL_STATE_UNBLOCKED. In your new patch the driver
will now clear STATUS_RF_KILL_SW, with STATUS_RF_KILL_HW still being
set. So, in this run, after iwl_rfkill_soft_rf_kill is called there will
be a state mismatch with rfkill thinking the system is unblocked while
the driver has it as hard blocked. This is not right.

Can this be fixed by adding a iwl_rfkill_set_hw_state in this run?

> > > - driver HW off
> > > Radio disabled - wrong 
> > > 
> > > Everything is fine when actions are in that order:
> > > 
> > > Radio enabled
> > > - rfkill SW on
> > > - driver HW on
> > > Radio disabled - ok 
> > > - driver HW off
> > > - rfkill SW off
> > > Radio enabled - ok 
> > 
> > 
> > Thanks for the explanation.
> > 
> > > 
> > > > > 
> > > > > Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> > > > > cause move to the same situation.
> > > > > 
> > > > > In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> > > > > This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> > > > > STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> > > > > subsystem SW switch bit before HW switch bit to move rfkill subsystem
> > > > > to SOFT_BLOCK rather than HARD_BLOCK.
> > > > > 
> > > > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > > > > ---
> > > > > I'm not sure if this is good candidate for stable as this is not backport
> > > > > of upstream commit. Also I did not test this patch with other iwlwifi devices,
> > > > > only with iwl3945.
> > > > > 
> > > > >  drivers/net/wireless/iwlwifi/iwl-rfkill.c |   24 ++++++++++++++----------
> > > > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > > index 2ad9faf..d6b6098 100644
> > > > > --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > > +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > > @@ -54,21 +54,28 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> > > > >  	case RFKILL_STATE_UNBLOCKED:
> > > > >  		if (iwl_is_rfkill_hw(priv)) {
> > > > >  			err = -EBUSY;
> > > > > -			goto out_unlock;
> > > > > +			/* pass error to rfkill core to make it state HARD
> > > > > +			 * BLOCKED and disable software kill switch */
> > > > >  		}
> > > > >  		iwl_radio_kill_sw_enable_radio(priv);
> > > > >  		break;
> > > > >  	case RFKILL_STATE_SOFT_BLOCKED:
> > > > >  		iwl_radio_kill_sw_disable_radio(priv);
> > > > > +		/* rfkill->mutex lock is taken */
> > > > > +		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> > > > > +			/* force rfkill core state to be SOFT BLOCKED,
> > > > > +			 * otherwise core will be unable to disable software
> > > > > +			 * kill switch */
> > > > > +			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> > > > > +		}
> > > > 
> > > > I understand that you are directly changing the rfkill internals because
> > > > the mutex is taken ... but this really does not seem right to directly
> > > > modify the rfkill state in this way.
> > > 
> > > Agree this is dirty hack, but I did not find a better way. Eventually,
> > > if we add call to rfkill_uevent(), this would behave the same
> > > as rfkill_force_state() .
> > 
> > Sorry, but I really do not understand why this code is needed. From what
> > you say rfkill can be in one of three states: RFKILL_STATE_UNBLOCKED,
> > RFKILL_STATE_SOFT_BLOCKED, or RFKILL_STATE_HARD_BLOCKED. From what I
> > understand the above code is called when there is an rfkill state change
> > and the new state is provided. So, only _one_ of the three states will
> > be provided as parameter. This state is then tested - so in the case
> > that you modified here the state has already been tested to be
> > RFKILL_STATE_SOFT_BLOCKED. How is it thus possible that it can be
> > RFKILL_STATE_HARD_BLOCKED also?
> 
> Local variable state != priv->rfkill->state . See rfkill_toggle_radio()
> especially this part:
> 
> 	if (force || state != rfkill->state) {
> 		retval = rfkill->toggle_radio(rfkill->data, state);
> 		/* never allow a HARD->SOFT downgrade! */

This comment makes me even more concerned about this patch. It
explicitly states "never allow a HARD->SOFT downgrade!" and that is what
your patch now seems to do.

> 		if (!retval && rfkill->state != RFKILL_STATE_HARD_BLOCKED)
> 			rfkill->state = state;
> 	}
> 
> Without the change rfkill core will be in state RFKILL_STATE_HARD_BLOCKED and
> latter will not clear STATE_RF_KILL_SW.
>  
> All hunks from the patch are needed on my laptop (lenoveo T60) to make
> killswitch works as expected. Applying only some hunks from the patch helps
> is one case or other, but without all hunks there is still possible to have
> radio disabled when killswitch is off.

>From what I can tell this patch introduced a disagreement of rfkill
state between driver and rfkill system. Maybe if we can sort this out we
do not need all these hunks?

Reinette

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[Stanislaw Gruszka]

Hello Reinette.

On Mon, Aug 10, 2009 at 09:44:52AM -0700, reinette chatre wrote:
> On Thu, 2009-08-06 at 23:31 -0700, Stanislaw Gruszka wrote:
> > On Thu, Aug 06, 2009 at 01:15:58PM -0700, reinette chatre wrote:
> > > On Thu, 2009-08-06 at 00:19 -0700, Stanislaw Gruszka wrote:
> > > > On Wed, Aug 05, 2009 at 03:51:49PM -0700, reinette chatre wrote:
> > > > > On Tue, 2009-08-04 at 05:35 -0700, Stanislaw Gruszka wrote:
> > > > > > Due to rfkill and iwlwifi mishmash of SW / HW killswitch representation,
> > > > > > we have race conditions which make unable turn wifi radio on, after enable
> > > > > > and disable again killswitch. I can observe this problem on my laptop
> > > > > > with iwl3945 device.
> > > > > > 
> > > > > > In rfkill core HW switch and SW switch are separate 'states'. Device can
> > > > > > be only in one of 3 states: RFKILL_STATE_SOFT_BLOCKED, RFKILL_STATE_UNBLOCKED,
> > > > > > RFKILL_STATE_HARD_BLOCKED. Whereas in iwlwifi driver we have separate bits
> > > > > > STATUS_RF_KILL_HW and STATUS_RF_KILL_SW for HW and SW switches - radio can be
> > > > > > turned on, only if both bits are cleared.
> > > > > > 
> > > > > > In this particular race conditions, radio can not be turned on if in driver
> > > > > > STATUS_RF_KILL_SW bit is set, and rfkill core is in state
> > > > > > RFKILL_STATE_HARD_BLOCKED, because rfkill core is unable to call
> > > > > > rfkill->toggle_radio(). This situation can be entered in case:
> > > > > > 
> > > > > 
> > > > > I am trying to understand this race condition ...
> > > > > 
> > > > > > - killswitch is turned on
> > > > > > - rfkill core 'see' button change first and move to RFKILL_STATE_SOFT_BLOCKED
> > > > > >   also call ->toggle_radio() and STATE_RF_KILL_SW in driver is set
> > > > > > - iwl3945 get info about button from hardware to set STATUS_RF_KILL_HW bit and
> > > > > >   force rfkill to move to RFKILL_STATE_HARD_BLOCKED
> > > > > 
> > > > > ok - so at this point we have rfkill == RFKILL_STATE_HARD_BLOCKED, and
> > > > > driver == STATE_RF_KILL_SW | STATE_RF_KILL_HW
> > > > > 
> > > > > > - killsiwtch is turend off
> > > > 
> > > > Here rfkill core routines are called. Rfkill wants to clear STATUS_RF_KILL_SW
> > > > but it can not as state is RFKILL_STATE_HARD_BLOCKED.
> > > > 
> > > > > > - driver clear STATUS_RF_KILL_HW
> > > > > 
> > > > > at this point the driver should clear STATE_RF_KILL_HW and then call
> > > > > iwl_rfkill_set_hw_state(). From what I can tell, in
> > > > > iwl_rfkill_set_hw_state() the test for iwl_is_rfkill_sw() will cause the
> > > > > driver to call rfkill_force_state for RFKILL_STATE_SOFT_BLOCKED
> > > > > 
> > > > > So, from what I understand after the above the status will be
> > > > > 
> > > > > rfkill == RFKILL_STATE_SOFT_BLOCKED, and driver == STATE_RF_KILL_SW 
> > > > 
> > > > Thats right. But rfkill core no longer wants to manipulate state via
> > > > ->toggle_radio() and radio stays disabled.
> > > >  
> > > > > > - rfkill core is unable to clear STATUS_RF_KILL_SW in driver
> > > > > 
> > > > > I do not understand why this is a problem here. Could you please
> > > > > highlight what I am missing?
> > > > 
> > > > In my description I miss the most important part, sorry. Race is when the
> > > > switches are performed in that order:
> > > > 
> > > > Radio enabled 
> > > > - rfkill SW on
> > > > - driver HW on
> > > > Radio disabled - ok
> > > > - rfkill SW off <- problem not clearing STATUS_RF_KILL_SW
> 
> Yes. I assume that what happens here is that rfkill notifies user that
> state changes to RFKILL_STATE_UNBLOCKED. In your new patch the driver
> will now clear STATUS_RF_KILL_SW, with STATUS_RF_KILL_HW still being
> set. So, in this run, after iwl_rfkill_soft_rf_kill is called there will
> be a state mismatch with rfkill thinking the system is unblocked while
> the driver has it as hard blocked. This is not right.

In such case we return -EBUSY from iwl_rfkill_soft_rf_kill() - rfkill
state not change. I made a comment it will be HARD_BLOCKED, this
is not true anymore, it can be also in state SOFT_BLOCKED . However
comment was true with previous version of the patch for 2.6.29, where
there was no HARD -> SOFT downgrade and that part was called only when 
rfkill state was HARD_BLOCKED.

> Can this be fixed by adding a iwl_rfkill_set_hw_state in this run?

We can not call iwl_rfkill_set_hw_state in iwl_rfkill_soft_rt_kill
as rfkill->muttex is taken. We eventually can force state in the same ugly
way as is done for case RFKILL_STATE_SOFT_BLOCKED and I think, this is good
idea :) , below not tested delta patch:

diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
index d6b6098..636c04a 100644
--- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
+++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
@@ -35,6 +35,19 @@
 #include "iwl-dev.h"
 #include "iwl-core.h"
 
+static void iwl_force_rfkill_state(struct iwl_priv *priv,
+				   enum rfkill_state state)
+{
+	enum rfkill_state oldstate;
+
+	oldstate = priv->rfkill->state;
+	priv->rfkill->state = state;
+
+	/* rfkill_uevent() */
+	if (oldstate != state)
+		kobject_uevent(&priv->rfkill->dev.kobj, KOBJ_CHANGE);
+}
+
 /* software rf-kill from user */
 static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
 {
@@ -54,8 +67,9 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
 	case RFKILL_STATE_UNBLOCKED:
 		if (iwl_is_rfkill_hw(priv)) {
 			err = -EBUSY;
-			/* pass error to rfkill core to make it state HARD
+			/* pass error to rfkill core, make it state HARD
 			 * BLOCKED and disable software kill switch */
+			iwl_force_rfkill_state(priv, RFKILL_STATE_HARD_BLOCKED);
 		}
 		iwl_radio_kill_sw_enable_radio(priv);
 		break;
@@ -63,10 +77,10 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
 		iwl_radio_kill_sw_disable_radio(priv);
 		/* rfkill->mutex lock is taken */
 		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
-			/* force rfkill core state to be SOFT BLOCKED,
+			/* force rfkill core state to be in SOFT BLOCKED,
 			 * otherwise core will be unable to disable software
 			 * kill switch */
-			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
+			iwl_force_rfkill_state(priv, RFKILL_STATE_SOFT_BLOCKED);
 		}
 		break;
 	default:
> > > > - driver HW off
> > > > Radio disabled - wrong 
> > > > 
> > > > Everything is fine when actions are in that order:
> > > > 
> > > > Radio enabled
> > > > - rfkill SW on
> > > > - driver HW on
> > > > Radio disabled - ok 
> > > > - driver HW off
> > > > - rfkill SW off
> > > > Radio enabled - ok 
> > > 
> > > 
> > > Thanks for the explanation.
> > > 
> > > > 
> > > > > > 
> > > > > > Additionally call to rfkill_epo() when STATUS_RF_KILL_HW in driver is set
> > > > > > cause move to the same situation.
> > > > > > 
> > > > > > In 2.6.31 this problem is fixed due to _total_ rewrite of rfkill subsystem.
> > > > > > This is a quite small fix for 2.6.30.x in iwl3945 driver. We disable
> > > > > > STATUS_RF_KILL_SW bit regardless of HW bit state. Also report to rfkill
> > > > > > subsystem SW switch bit before HW switch bit to move rfkill subsystem
> > > > > > to SOFT_BLOCK rather than HARD_BLOCK.
> > > > > > 
> > > > > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > > > > > ---
> > > > > > I'm not sure if this is good candidate for stable as this is not backport
> > > > > > of upstream commit. Also I did not test this patch with other iwlwifi devices,
> > > > > > only with iwl3945.
> > > > > > 
> > > > > >  drivers/net/wireless/iwlwifi/iwl-rfkill.c |   24 ++++++++++++++----------
> > > > > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > > > index 2ad9faf..d6b6098 100644
> > > > > > --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > > > +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> > > > > > @@ -54,21 +54,28 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> > > > > >  	case RFKILL_STATE_UNBLOCKED:
> > > > > >  		if (iwl_is_rfkill_hw(priv)) {
> > > > > >  			err = -EBUSY;
> > > > > > -			goto out_unlock;
> > > > > > +			/* pass error to rfkill core to make it state HARD
> > > > > > +			 * BLOCKED and disable software kill switch */
> > > > > >  		}
> > > > > >  		iwl_radio_kill_sw_enable_radio(priv);
> > > > > >  		break;
> > > > > >  	case RFKILL_STATE_SOFT_BLOCKED:
> > > > > >  		iwl_radio_kill_sw_disable_radio(priv);
> > > > > > +		/* rfkill->mutex lock is taken */
> > > > > > +		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> > > > > > +			/* force rfkill core state to be SOFT BLOCKED,
> > > > > > +			 * otherwise core will be unable to disable software
> > > > > > +			 * kill switch */
> > > > > > +			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> > > > > > +		}
> > > > > 
> > > > > I understand that you are directly changing the rfkill internals because
> > > > > the mutex is taken ... but this really does not seem right to directly
> > > > > modify the rfkill state in this way.
> > > > 
> > > > Agree this is dirty hack, but I did not find a better way. Eventually,
> > > > if we add call to rfkill_uevent(), this would behave the same
> > > > as rfkill_force_state() .
> > > 
> > > Sorry, but I really do not understand why this code is needed. From what
> > > you say rfkill can be in one of three states: RFKILL_STATE_UNBLOCKED,
> > > RFKILL_STATE_SOFT_BLOCKED, or RFKILL_STATE_HARD_BLOCKED. From what I
> > > understand the above code is called when there is an rfkill state change
> > > and the new state is provided. So, only _one_ of the three states will
> > > be provided as parameter. This state is then tested - so in the case
> > > that you modified here the state has already been tested to be
> > > RFKILL_STATE_SOFT_BLOCKED. How is it thus possible that it can be
> > > RFKILL_STATE_HARD_BLOCKED also?
> > 
> > Local variable state != priv->rfkill->state . See rfkill_toggle_radio()
> > especially this part:
> > 
> > 	if (force || state != rfkill->state) {
> > 		retval = rfkill->toggle_radio(rfkill->data, state);
> > 		/* never allow a HARD->SOFT downgrade! */
> 
> This comment makes me even more concerned about this patch. It
> explicitly states "never allow a HARD->SOFT downgrade!" and that is what
> your patch now seems to do.

Yes, but that was rewritten in upstream ...

> > 		if (!retval && rfkill->state != RFKILL_STATE_HARD_BLOCKED)
> > 			rfkill->state = state;
> > 	}
> > 
> > Without the change rfkill core will be in state RFKILL_STATE_HARD_BLOCKED and
> > latter will not clear STATE_RF_KILL_SW.
> >  
> > All hunks from the patch are needed on my laptop (lenoveo T60) to make
> > killswitch works as expected. Applying only some hunks from the patch helps
> > is one case or other, but without all hunks there is still possible to have
> > radio disabled when killswitch is off.
> 
> >From what I can tell this patch introduced a disagreement of rfkill
> state between driver and rfkill system. 

In driver we have no states, but separate bits for each killswitch. Situation
gets better after rfkill rewrite where also rfkill core become to have separate
bits, but with 2.6.30 we have no such luck. 

Currently we have "states" like below: 

STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_HARD_BLOCKED
STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED

Patch is intended to work like that:

STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED

> Maybe if we can sort this out we do not need all these hunks?

Sort out ... uff, it's hard to explain all possible thinks that
can happen here - ok let's try.

RH bugzilla report is here:

http://bugzilla.redhat.com/show_bug.cgi?id=498622

Report was originally for fedora version of 2.6.29 and patch without ugly force
HARD->SOFT downgrade, fix the bug. However in vanilla 2.6.30.4 problem is worse
due to some changes to input (or rfkill-input) layer. Except above ordering,
which can make radio disabled when killswitch is off, similarly things can be
done is such order:

  STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

driver HW on

  STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED

rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)

  STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED

rfkill SW off (HARD_BLOCKED not clearing STATUS_RF_KILL_SW)  

  STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED

driver HW off (called from iwl_bg_rf_kill())

  STATUS_RF_KILL_HW=0, STATUS_RF_KILL_SW=1, RFKILL_STATE_SOFT_BLOCKED

rfkill core no longer wants to turn radio on

I guess problems only happens on iwl3945 due to HW killswitch is
checked from workqueue in polling way. If this would be interrupt we
will probably not encounter races.

Cheers
Stanislaw 

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[reinette chatre]

Hi Stanislaw,

Thank you for your patience ...

On Tue, 2009-08-11 at 07:09 -0700, Stanislaw Gruszka wrote:
> On Mon, Aug 10, 2009 at 09:44:52AM -0700, reinette chatre wrote:
> > Yes. I assume that what happens here is that rfkill notifies user that
> > state changes to RFKILL_STATE_UNBLOCKED. In your new patch the driver
> > will now clear STATUS_RF_KILL_SW, with STATUS_RF_KILL_HW still being
> > set. So, in this run, after iwl_rfkill_soft_rf_kill is called there will
> > be a state mismatch with rfkill thinking the system is unblocked while
> > the driver has it as hard blocked. This is not right.
> 
> In such case we return -EBUSY from iwl_rfkill_soft_rf_kill() - rfkill
> state not change. 

oh - right - sorry

> I made a comment it will be HARD_BLOCKED, this
> is not true anymore, it can be also in state SOFT_BLOCKED . 

How so? Isn't the idea behind toggle_radio that the SOFT_BLOCKED state
changes? In this case when we get a new state of UNBLOCKED then I do not
understand how SOFT_BLOCKED can be true also.

> However
> comment was true with previous version of the patch for 2.6.29, where
> there was no HARD -> SOFT downgrade and that part was called only when
> rfkill state was HARD_BLOCKED.
> 
> > Can this be fixed by adding a iwl_rfkill_set_hw_state in this run?
> 
> We can not call iwl_rfkill_set_hw_state in iwl_rfkill_soft_rt_kill
> as rfkill->muttex is taken. We eventually can force state in the same ugly
> way as is done for case RFKILL_STATE_SOFT_BLOCKED and I think, this is good
> idea :) , below not tested delta patch:
> 

This just seems to mess with the rfkill internals even more. Can this
not be avoided?

> diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> index d6b6098..636c04a 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> @@ -35,6 +35,19 @@
>  #include "iwl-dev.h"
>  #include "iwl-core.h"
> 
> +static void iwl_force_rfkill_state(struct iwl_priv *priv,
> +                                  enum rfkill_state state)
> +{
> +       enum rfkill_state oldstate;
> +
> +       oldstate = priv->rfkill->state;
> +       priv->rfkill->state = state;
> +
> +       /* rfkill_uevent() */
> +       if (oldstate != state)
> +               kobject_uevent(&priv->rfkill->dev.kobj, KOBJ_CHANGE);
> +}
> +
>  /* software rf-kill from user */
>  static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
>  {
> @@ -54,8 +67,9 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
>         case RFKILL_STATE_UNBLOCKED:
>                 if (iwl_is_rfkill_hw(priv)) {
>                         err = -EBUSY;
> -                       /* pass error to rfkill core to make it state HARD
> +                       /* pass error to rfkill core, make it state HARD
>                          * BLOCKED and disable software kill switch */
> +                       iwl_force_rfkill_state(priv, RFKILL_STATE_HARD_BLOCKED);
>                 }
>                 iwl_radio_kill_sw_enable_radio(priv);
>                 break;
> @@ -63,10 +77,10 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
>                 iwl_radio_kill_sw_disable_radio(priv);
>                 /* rfkill->mutex lock is taken */
>                 if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> -                       /* force rfkill core state to be SOFT BLOCKED,
> +                       /* force rfkill core state to be in SOFT BLOCKED,
>                          * otherwise core will be unable to disable software
>                          * kill switch */
> -                       priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> +                       iwl_force_rfkill_state(priv, RFKILL_STATE_SOFT_BLOCKED);
>                 }
>                 break;
>         default:

> > >From what I can tell this patch introduced a disagreement of rfkill
> > state between driver and rfkill system.
> 
> In driver we have no states, but separate bits for each killswitch. Situation
> gets better after rfkill rewrite where also rfkill core become to have separate
> bits, but with 2.6.30 we have no such luck.
> 
> Currently we have "states" like below:
> 
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_HARD_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED
> 
> Patch is intended to work like that:
> 
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED

I can see that this is what the last hunk of the patch accomplishes -
but I do not see why it is needed.

> 
>   STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> 
> driver HW on
> 
>   STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> 
> rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)
> 
>   STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED
> 
> rfkill SW off (HARD_BLOCKED not clearing STATUS_RF_KILL_SW)
> 
>   STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED
> 
> driver HW off (called from iwl_bg_rf_kill())
> 
>   STATUS_RF_KILL_HW=0, STATUS_RF_KILL_SW=1, RFKILL_STATE_SOFT_BLOCKED
> 
> rfkill core no longer wants to turn radio on

>From what I understand what you are describing above should be addressed
by this hunk of your patch:

        case RFKILL_STATE_UNBLOCKED:
                if (iwl_is_rfkill_hw(priv)) {
                        err = -EBUSY;
-                       goto out_unlock;
+                       /* pass error to rfkill core to make it state HARD
+                        * BLOCKED and disable software kill switch */
                }

This should make these new transitions possible:

	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
driver HW on
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
rfkill SW off 
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
driver HW off (called from iwl_bg_rf_kill())
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED


Looking further I tried to see how other combinations would be treated. Here is how I see the potential scenarios:

Case1 (considered above):
driver HW on -> rfkill SW on -> rfkill SW off -> driver HW off
Case2:
driver HW on -> rfkill SW on -> driver HW off -> rfkill SW off
Case3:
rfkill SW on -> driver HW on -> rfkill SW off -> driver HW off
Case4:
rfkill SW on -> driver HW on -> driver HW off -> rfkill SW off 


Looking at the rest of the cases I do not see the problem addressed by the other hunks.

I see:

Case 2:
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
driver HW on
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
rfkill SW on
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
driver HW off
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
rfkill SW off 
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

Case3:
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
rfkill SW on
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
driver HW on
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
rfkill SW off
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
driver HW off
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

Case4:
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
rfkill SW on
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
driver HW on
	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
driver HW off
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
rfkill SW off 
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

I understand that one hunk of your patch accomplishes the mapping of
"STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <->
RFKILL_STATUS_SOFT_BLOCKED" - but I do not understand why it is needed. Could you please explain?

I also do not understand the need to modify rfkill's internal state.

Reinette

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[Stanislaw Gruszka]

On Tue, Aug 11, 2009 at 11:08:33AM -0700, reinette chatre wrote:
> Hi Stanislaw,
> 
> Thank you for your patience ...

Hello, I understand your concerns, patch is not so straightforward and
hard to understand, if you don't have system where you can reproduce.

> On Tue, 2009-08-11 at 07:09 -0700, Stanislaw Gruszka wrote:
> > On Mon, Aug 10, 2009 at 09:44:52AM -0700, reinette chatre wrote:
> > > Yes. I assume that what happens here is that rfkill notifies user that
> > > state changes to RFKILL_STATE_UNBLOCKED. In your new patch the driver
> > > will now clear STATUS_RF_KILL_SW, with STATUS_RF_KILL_HW still being
> > > set. So, in this run, after iwl_rfkill_soft_rf_kill is called there will
> > > be a state mismatch with rfkill thinking the system is unblocked while
> > > the driver has it as hard blocked. This is not right.
> > 
> > In such case we return -EBUSY from iwl_rfkill_soft_rf_kill() - rfkill
> > state not change. 
> 
> oh - right - sorry
> 
> > I made a comment it will be HARD_BLOCKED, this
> > is not true anymore, it can be also in state SOFT_BLOCKED . 
> 
> How so? Isn't the idea behind toggle_radio that the SOFT_BLOCKED state
> changes? In this case when we get a new state of UNBLOCKED then I do not
> understand how SOFT_BLOCKED can be true also.

Hugh, right I was completely wrong here. 
 
> > However
> > comment was true with previous version of the patch for 2.6.29, where
> > there was no HARD -> SOFT downgrade and that part was called only when
> > rfkill state was HARD_BLOCKED.
> > 
> > > Can this be fixed by adding a iwl_rfkill_set_hw_state in this run?
> > 
> > We can not call iwl_rfkill_set_hw_state in iwl_rfkill_soft_rt_kill
> > as rfkill->muttex is taken. We eventually can force state in the same ugly
> > way as is done for case RFKILL_STATE_SOFT_BLOCKED and I think, this is good
> > idea :) , below not tested delta patch:
> > 
> 
> This just seems to mess with the rfkill internals even more. Can this
> not be avoided?

Other solution eventually would be ignore rfkill core request to SW disable
radio when we have already STATUS_RF_KILL_HW=1, but I think it is very bad
idea and probably broke thinks.

We currently call rfkill_force_state() which is changing internal state
of rfkill core, however it is done through defined api. Uh, patch is
not ideal, but I do not have anything better. 

> > > >From what I can tell this patch introduced a disagreement of rfkill
> > > state between driver and rfkill system.
> > 
> > In driver we have no states, but separate bits for each killswitch. Situation
> > gets better after rfkill rewrite where also rfkill core become to have separate
> > bits, but with 2.6.30 we have no such luck.
> > 
> > Currently we have "states" like below:
> > 
> > STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_HARD_BLOCKED
> > STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> > STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
> > STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED
> > 
> > Patch is intended to work like that:
> > 
> > STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> > STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> > STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
> > STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED
> 
> I can see that this is what the last hunk of the patch accomplishes -
> but I do not see why it is needed.
> 
> > 
> >   STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> > 
> > driver HW on
> > 
> >   STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> > 
> > rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)
> > 
> >   STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED
> > 
> > rfkill SW off (HARD_BLOCKED not clearing STATUS_RF_KILL_SW)
> > 
> >   STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED
> > 
> > driver HW off (called from iwl_bg_rf_kill())
> > 
> >   STATUS_RF_KILL_HW=0, STATUS_RF_KILL_SW=1, RFKILL_STATE_SOFT_BLOCKED
> > 
> > rfkill core no longer wants to turn radio on
> 
> >From what I understand what you are describing above should be addressed
> by this hunk of your patch:
> 
>         case RFKILL_STATE_UNBLOCKED:
>                 if (iwl_is_rfkill_hw(priv)) {
>                         err = -EBUSY;
> -                       goto out_unlock;
> +                       /* pass error to rfkill core to make it state HARD
> +                        * BLOCKED and disable software kill switch */
>                 }
> 
> This should make these new transitions possible:
> 
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> driver HW on
> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)
> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
> rfkill SW off 
> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
No, rfkill core will not call ->toggle_radio()
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
> driver HW off (called from iwl_bg_rf_kill())
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
Would be:
 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_UNBLOCKED

Not work without the patch, with patch it works like that:

 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
driver HW on
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
rfkill SW on 
	rfkill call -> rfkill_epo() -> rfkill_toggle_radio(RFKILL_STATE_SOFT_BLOCKED)
	with force = 1 . Due to changes in iwl_rfkill_soft_rf_kill() we move
	state to RFKILL_STATE_SOFT_BLOCKED, so:
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
rfkill SW off 
	rfkill core call ->toggle_radio(RFKILL_STATE_UNBLOCKED) 
	iwl_is_rfkill_hw(priv) is true but we disable STATUS_RF_KILL_SW
	anyway and return -EBUSY to not change rfkill core state, so:
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_SOFT_BLOCKED
driver HW off
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

> Looking further I tried to see how other combinations would be treated. Here is how I see the potential scenarios:
> 
> Case1 (considered above):
> driver HW on -> rfkill SW on -> rfkill SW off -> driver HW off
> Case2:
> driver HW on -> rfkill SW on -> driver HW off -> rfkill SW off
> Case3:
> rfkill SW on -> driver HW on -> rfkill SW off -> driver HW off
> Case4:
> rfkill SW on -> driver HW on -> driver HW off -> rfkill SW off 
> 
> Looking at the rest of the cases I do not see the problem addressed by the other hunks.
> 
> I see:
> 
> Case 2:
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> driver HW on
> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> rfkill SW on
> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
> driver HW off
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
> rfkill SW off 
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

Yes, works without the patch.

> Case3:
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> rfkill SW on
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
> driver HW on
> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
> rfkill SW off

No, rfkill will not call ->toggle_radio()  

> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> driver HW off
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

Not work without the patch, with patch it works like that:

Case3:
 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
rfkill SW on
 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
driver HW on
	Here due to changes in iwl_rfkill_set_hw_state() rfkill core stay in
	RFKILL_STATE_SOFT_BLOCKED so:
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
rfkill SW off
	rfkill core call ->toggle_radio(RFKILL_STATE_UNBLOCKED) 
	iwl_is_rfkill_hw(priv) is true but we disable STATUS_RF_KILL_SW
	anyway and return -EBUSY to not change rfkill core state, so:
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_SOFT_BLOCKED
driver HW off
	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

> Case4:
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> rfkill SW on
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
> driver HW on
> 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
> driver HW off
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
> rfkill SW off 
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> 

Yes, work without the patch.

> I understand that one hunk of your patch accomplishes the mapping of
> "STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <->
> RFKILL_STATUS_SOFT_BLOCKED" - but I do not understand why it is needed. Could you please explain?

I hope above explanation are clear now. 

> I also do not understand the need to modify rfkill's internal state.

It's needed for Case1. Additional change of internal rfkill state, which
I proposed in previous e-mail is against situation when we have:
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_SOFT_BLOCKED
To make it:
 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED

Regards
Stanislaw

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[reinette chatre]

Hi Stanislaw,

On Wed, 2009-08-12 at 08:12 -0700, Stanislaw Gruszka wrote:
> On Tue, Aug 11, 2009 at 11:08:33AM -0700, reinette chatre wrote:

>  
> > 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> > driver HW on
> > 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> > rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)
> > 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
> > rfkill SW off 
> > 	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> No, rfkill core will not call ->toggle_radio()

oh ... I see .... in rfkill_toggle_radio -EPERM is returned in this
case.

>  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
> > driver HW off (called from iwl_bg_rf_kill())
> > 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> Would be:
>  	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_UNBLOCKED
> 
> Not work without the patch, with patch it works like that:
> 
>  	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
> driver HW on
>  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> rfkill SW on 
> 	rfkill call -> rfkill_epo() -> rfkill_toggle_radio(RFKILL_STATE_SOFT_BLOCKED)
> 	with force = 1 . Due to changes in iwl_rfkill_soft_rf_kill() we move
> 	state to RFKILL_STATE_SOFT_BLOCKED, so:
>  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
> rfkill SW off 
> 	rfkill core call ->toggle_radio(RFKILL_STATE_UNBLOCKED) 
> 	iwl_is_rfkill_hw(priv) is true but we disable STATUS_RF_KILL_SW
> 	anyway and return -EBUSY to not change rfkill core state, so:
>  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_SOFT_BLOCKED
> driver HW off
> 	STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED

I see ... the state mismatch is a bit strange, but you talk about that
later ...


> > I understand that one hunk of your patch accomplishes the mapping of
> > "STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <->
> > RFKILL_STATUS_SOFT_BLOCKED" - but I do not understand why it is needed. Could you please explain?
> 
> I hope above explanation are clear now. 

yes - thank you very much.

> 
> > I also do not understand the need to modify rfkill's internal state.
> 
> It's needed for Case1. Additional change of internal rfkill state, which
> I proposed in previous e-mail is against situation when we have:
>  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_SOFT_BLOCKED
> To make it:
>  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED

ok - this makes sense now. In your previous email you also mentioned
that that delta patch was untested. Is it possible for you or anybody
else on that redhat bugzilla to give the new patch a try?

I think I now understand what is going on. Having worked through all the
possible scenarios makes me more comfortable about his patch considering
the awkward way in which it is forced to solve the problem. I am really
glad we do not need to do this moving forward.

Reinette

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[Stanislaw Gruszka]

On Wed, Aug 12, 2009 at 09:45:02AM -0700, reinette chatre wrote:
> > > I also do not understand the need to modify rfkill's internal state.
> > 
> > It's needed for Case1. Additional change of internal rfkill state, which
> > I proposed in previous e-mail is against situation when we have:
> >  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_SOFT_BLOCKED
> > To make it:
> >  	STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
> 
> ok - this makes sense now. In your previous email you also mentioned
> that that delta patch was untested. Is it possible for you or anybody
> else on that redhat bugzilla to give the new patch a try?
Yes, I'm going to rewrite patch, test and resend it. 
 
> I think I now understand what is going on. Having worked through all the
> possible scenarios makes me more comfortable about his patch considering
> the awkward way in which it is forced to solve the problem. I am really
> glad we do not need to do this moving forward.
I'm happy too.

Thanks
Stanislaw

Re: [PATCH 2.6.30] iwl3945: fix rfkill switch

[Stanislaw Gruszka]

On Tue, Aug 11, 2009 at 04:09:08PM +0200, Stanislaw Gruszka wrote:
> diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> index d6b6098..636c04a 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> @@ -35,6 +35,19 @@
>  #include "iwl-dev.h"
>  #include "iwl-core.h"
>  
> +static void iwl_force_rfkill_state(struct iwl_priv *priv,
> +				   enum rfkill_state state)
> +{
> +	enum rfkill_state oldstate;
> +
> +	oldstate = priv->rfkill->state;
> +	priv->rfkill->state = state;
> +
> +	/* rfkill_uevent() */
> +	if (oldstate != state)
> +		kobject_uevent(&priv->rfkill->dev.kobj, KOBJ_CHANGE);

This is not needed because rfkill_toggle_radio() will send
rfkill_uevent().
  
> +}
> +
>  /* software rf-kill from user */
>  static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
>  {
> @@ -54,8 +67,9 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
>  	case RFKILL_STATE_UNBLOCKED:
>  		if (iwl_is_rfkill_hw(priv)) {
>  			err = -EBUSY;
> -			/* pass error to rfkill core to make it state HARD
> +			/* pass error to rfkill core, make it state HARD
>  			 * BLOCKED and disable software kill switch */
> +			iwl_force_rfkill_state(priv, RFKILL_STATE_HARD_BLOCKED);
>  		}
>  		iwl_radio_kill_sw_enable_radio(priv);
>  		break;
> @@ -63,10 +77,10 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
>  		iwl_radio_kill_sw_disable_radio(priv);
>  		/* rfkill->mutex lock is taken */
>  		if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> -			/* force rfkill core state to be SOFT BLOCKED,
> +			/* force rfkill core state to be in SOFT BLOCKED,
>  			 * otherwise core will be unable to disable software
>  			 * kill switch */
> -			priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> +			iwl_force_rfkill_state(priv, RFKILL_STATE_SOFT_BLOCKED);
>  		}
>  		break;

Stanislaw