[GIT PATCH] rfkill fixes, set 3

[GIT PATCH] rfkill fixes, set 3

[Henrique de Moraes Holschuh]

This is a small set of two patches that fix issues in current rfkill.

If Ivo acks them, please merge on wireless-testing, and after a small
while, please consider sending them to mainline for 2.6.27-rc merging.

Henrique de Moraes Holschuh (2):
      rfkill: protect suspended rfkill controllers
      rfkill: use strict_strtoul

Thanks!

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

[PATCH 1/2] rfkill: protect suspended rfkill controllers

[Henrique de Moraes Holschuh]

Guard rfkill controllers attached to a rfkill class against state changes
after class suspend has been issued.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
---
 Documentation/rfkill.txt |    5 +++++
 net/rfkill/rfkill.c      |   14 ++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index 28b6ec8..6fcb306 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -363,6 +363,11 @@ This rule exists because users of the rfkill subsystem expect to get (and set,
 when possible) the overall transmitter rfkill state, not of a particular rfkill
 line.
 
+5. During suspend, the rfkill class will attempt to soft-block the radio
+through a call to rfkill->toggle_radio, and will try to restore its previous
+state during resume.  After a rfkill class is suspended, it will *not* call
+rfkill->toggle_radio until it is resumed.
+
 Example of a WLAN wireless driver connected to the rfkill subsystem:
 --------------------------------------------------------------------
 
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index d2d4565..35a9994 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -150,6 +150,8 @@ static void update_rfkill_state(struct rfkill *rfkill)
  * calls and handling all the red tape such as issuing notifications
  * if the call is successful.
  *
+ * Suspended devices are not touched at all, and -EAGAIN is returned.
+ *
  * Note that the @force parameter cannot override a (possibly cached)
  * state of RFKILL_STATE_HARD_BLOCKED.  Any device making use of
  * RFKILL_STATE_HARD_BLOCKED implements either get_state() or
@@ -168,6 +170,9 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
 	int retval = 0;
 	enum rfkill_state oldstate, newstate;
 
+	if (unlikely(rfkill->dev.power.power_state.event & PM_EVENT_SLEEP))
+		return -EBUSY;
+
 	oldstate = rfkill->state;
 
 	if (rfkill->get_state && !force &&
@@ -214,7 +219,7 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
  *
  * This function toggles the state of all switches of given type,
  * unless a specific switch is claimed by userspace (in which case,
- * that switch is left alone).
+ * that switch is left alone) or suspended.
  */
 void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
 {
@@ -239,8 +244,8 @@ EXPORT_SYMBOL(rfkill_switch_all);
 /**
  * rfkill_epo - emergency power off all transmitters
  *
- * This kicks all rfkill devices to RFKILL_STATE_SOFT_BLOCKED, ignoring
- * everything in its path but rfkill_mutex and rfkill->mutex.
+ * This kicks all non-suspended rfkill devices to RFKILL_STATE_SOFT_BLOCKED,
+ * ignoring everything in its path but rfkill_mutex and rfkill->mutex.
  */
 void rfkill_epo(void)
 {
@@ -458,13 +463,14 @@ static int rfkill_resume(struct device *dev)
 	if (dev->power.power_state.event != PM_EVENT_ON) {
 		mutex_lock(&rfkill->mutex);
 
+		dev->power.power_state.event = PM_EVENT_ON;
+
 		/* restore radio state AND notify everybody */
 		rfkill_toggle_radio(rfkill, rfkill->state, 1);
 
 		mutex_unlock(&rfkill->mutex);
 	}
 
-	dev->power.power_state = PMSG_ON;
 	return 0;
 }
 #else
-- 
1.5.6.3

Re: [PATCH 1/2] rfkill: protect suspended rfkill controllers

[Ivo van Doorn]

On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> Guard rfkill controllers attached to a rfkill class against state changes
> after class suspend has been issued.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  Documentation/rfkill.txt |    5 +++++
>  net/rfkill/rfkill.c      |   14 ++++++++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
> index 28b6ec8..6fcb306 100644
> --- a/Documentation/rfkill.txt
> +++ b/Documentation/rfkill.txt
> @@ -363,6 +363,11 @@ This rule exists because users of the rfkill subsystem expect to get (and set,
>  when possible) the overall transmitter rfkill state, not of a particular rfkill
>  line.
>  
> +5. During suspend, the rfkill class will attempt to soft-block the radio
> +through a call to rfkill->toggle_radio, and will try to restore its previous
> +state during resume.  After a rfkill class is suspended, it will *not* call
> +rfkill->toggle_radio until it is resumed.
> +
>  Example of a WLAN wireless driver connected to the rfkill subsystem:
>  --------------------------------------------------------------------
>  
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index d2d4565..35a9994 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -150,6 +150,8 @@ static void update_rfkill_state(struct rfkill *rfkill)
>   * calls and handling all the red tape such as issuing notifications
>   * if the call is successful.
>   *
> + * Suspended devices are not touched at all, and -EAGAIN is returned.
> + *
>   * Note that the @force parameter cannot override a (possibly cached)
>   * state of RFKILL_STATE_HARD_BLOCKED.  Any device making use of
>   * RFKILL_STATE_HARD_BLOCKED implements either get_state() or
> @@ -168,6 +170,9 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
>  	int retval = 0;
>  	enum rfkill_state oldstate, newstate;
>  
> +	if (unlikely(rfkill->dev.power.power_state.event & PM_EVENT_SLEEP))
> +		return -EBUSY;
> +
>  	oldstate = rfkill->state;
>  
>  	if (rfkill->get_state && !force &&
> @@ -214,7 +219,7 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
>   *
>   * This function toggles the state of all switches of given type,
>   * unless a specific switch is claimed by userspace (in which case,
> - * that switch is left alone).
> + * that switch is left alone) or suspended.
>   */
>  void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
>  {
> @@ -239,8 +244,8 @@ EXPORT_SYMBOL(rfkill_switch_all);
>  /**
>   * rfkill_epo - emergency power off all transmitters
>   *
> - * This kicks all rfkill devices to RFKILL_STATE_SOFT_BLOCKED, ignoring
> - * everything in its path but rfkill_mutex and rfkill->mutex.
> + * This kicks all non-suspended rfkill devices to RFKILL_STATE_SOFT_BLOCKED,
> + * ignoring everything in its path but rfkill_mutex and rfkill->mutex.
>   */
>  void rfkill_epo(void)
>  {
> @@ -458,13 +463,14 @@ static int rfkill_resume(struct device *dev)
>  	if (dev->power.power_state.event != PM_EVENT_ON) {
>  		mutex_lock(&rfkill->mutex);
>  
> +		dev->power.power_state.event = PM_EVENT_ON;
> +
>  		/* restore radio state AND notify everybody */
>  		rfkill_toggle_radio(rfkill, rfkill->state, 1);
>  
>  		mutex_unlock(&rfkill->mutex);
>  	}
>  
> -	dev->power.power_state = PMSG_ON;
>  	return 0;
>  }
>  #else

[PATCH 2/2] rfkill: use strict_strtoul

[Henrique de Moraes Holschuh]

Switch sysfs parsing to something that actually works properly.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
---
 net/rfkill/rfkill.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 35a9994..2ec6312 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -352,12 +352,16 @@ static ssize_t rfkill_state_store(struct device *dev,
 				  const char *buf, size_t count)
 {
 	struct rfkill *rfkill = to_rfkill(dev);
-	unsigned int state = simple_strtoul(buf, NULL, 0);
+	unsigned long state;
 	int error;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
+	error = strict_strtoul(buf, 0, &state);
+	if (error)
+		return error;
+
 	/* RFKILL_STATE_HARD_BLOCKED is illegal here... */
 	if (state != RFKILL_STATE_UNBLOCKED &&
 	    state != RFKILL_STATE_SOFT_BLOCKED)
@@ -385,7 +389,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
 				  const char *buf, size_t count)
 {
 	struct rfkill *rfkill = to_rfkill(dev);
-	bool claim = !!simple_strtoul(buf, NULL, 0);
+	unsigned long claim;
 	int error;
 
 	if (!capable(CAP_NET_ADMIN))
@@ -394,6 +398,10 @@ static ssize_t rfkill_claim_store(struct device *dev,
 	if (rfkill->user_claim_unsupported)
 		return -EOPNOTSUPP;
 
+	error = strict_strtoul(buf, 0, &claim);
+	if (error)
+		return error;
+
 	/*
 	 * Take the global lock to make sure the kernel is not in
 	 * the middle of rfkill_switch_all
@@ -402,7 +410,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
 	if (error)
 		return error;
 
-	if (rfkill->user_claim != claim) {
+	if (!!rfkill->user_claim != !!claim) {
 		if (!claim) {
 			mutex_lock(&rfkill->mutex);
 			rfkill_toggle_radio(rfkill,
@@ -410,7 +418,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
 					    0);
 			mutex_unlock(&rfkill->mutex);
 		}
-		rfkill->user_claim = claim;
+		rfkill->user_claim = !!claim;
 	}
 
 	mutex_unlock(&rfkill_mutex);
-- 
1.5.6.3

Re: [PATCH 2/2] rfkill: use strict_strtoul

[Ivo van Doorn]

On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> Switch sysfs parsing to something that actually works properly.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  net/rfkill/rfkill.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 35a9994..2ec6312 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -352,12 +352,16 @@ static ssize_t rfkill_state_store(struct device *dev,
>  				  const char *buf, size_t count)
>  {
>  	struct rfkill *rfkill = to_rfkill(dev);
> -	unsigned int state = simple_strtoul(buf, NULL, 0);
> +	unsigned long state;
>  	int error;
>  
>  	if (!capable(CAP_NET_ADMIN))
>  		return -EPERM;
>  
> +	error = strict_strtoul(buf, 0, &state);
> +	if (error)
> +		return error;
> +
>  	/* RFKILL_STATE_HARD_BLOCKED is illegal here... */
>  	if (state != RFKILL_STATE_UNBLOCKED &&
>  	    state != RFKILL_STATE_SOFT_BLOCKED)
> @@ -385,7 +389,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
>  				  const char *buf, size_t count)
>  {
>  	struct rfkill *rfkill = to_rfkill(dev);
> -	bool claim = !!simple_strtoul(buf, NULL, 0);
> +	unsigned long claim;
>  	int error;
>  
>  	if (!capable(CAP_NET_ADMIN))
> @@ -394,6 +398,10 @@ static ssize_t rfkill_claim_store(struct device *dev,
>  	if (rfkill->user_claim_unsupported)
>  		return -EOPNOTSUPP;
>  
> +	error = strict_strtoul(buf, 0, &claim);
> +	if (error)
> +		return error;
> +
>  	/*
>  	 * Take the global lock to make sure the kernel is not in
>  	 * the middle of rfkill_switch_all
> @@ -402,7 +410,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
>  	if (error)
>  		return error;
>  
> -	if (rfkill->user_claim != claim) {
> +	if (!!rfkill->user_claim != !!claim) {
>  		if (!claim) {
>  			mutex_lock(&rfkill->mutex);
>  			rfkill_toggle_radio(rfkill,
> @@ -410,7 +418,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
>  					    0);
>  			mutex_unlock(&rfkill->mutex);
>  		}
> -		rfkill->user_claim = claim;
> +		rfkill->user_claim = !!claim;
>  	}
>  
>  	mutex_unlock(&rfkill_mutex);

Re: [PATCH 2/2] rfkill: use strict_strtoul

[John W. Linville]

On Sat, Aug 02, 2008 at 02:56:26PM -0300, Henrique de Moraes Holschuh wrote:
> Switch sysfs parsing to something that actually works properly.

> @@ -402,7 +410,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
>  	if (error)
>  		return error;
>  
> -	if (rfkill->user_claim != claim) {
> +	if (!!rfkill->user_claim != !!claim) {
>  		if (!claim) {
>  			mutex_lock(&rfkill->mutex);
>  			rfkill_toggle_radio(rfkill,

This looks a bit funny.  Is the '!!' in front of 'rfkill->user_claim'
really necessary?  Especially since...

> @@ -410,7 +418,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
>  					    0);
>  			mutex_unlock(&rfkill->mutex);
>  		}
> -		rfkill->user_claim = claim;
> +		rfkill->user_claim = !!claim;
>  	}
>  
>  	mutex_unlock(&rfkill_mutex);

You seem to be doing the only assignment to 'rfkill->user_claim',
using a '!!' to condition the input?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH 2/2] rfkill: use strict_strtoul

[Michael Buesch]

On Sunday 17 August 2008 20:16:17 John W. Linville wrote:
> On Sat, Aug 02, 2008 at 02:56:26PM -0300, Henrique de Moraes Holschuh wrote:
> > Switch sysfs parsing to something that actually works properly.
> 
> > @@ -402,7 +410,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
> >  	if (error)
> >  		return error;
> >  
> > -	if (rfkill->user_claim != claim) {
> > +	if (!!rfkill->user_claim != !!claim) {
> >  		if (!claim) {
> >  			mutex_lock(&rfkill->mutex);
> >  			rfkill_toggle_radio(rfkill,
> 
> This looks a bit funny.  Is the '!!' in front of 'rfkill->user_claim'
> really necessary?  Especially since...

You can remove the !! in front of rfkill->user_claim, because rfkill->user_claim
is bool anyway. The compiler will implicitely apply !! to it where it is needed.

-- 
Greetings Michael.

Re: [PATCH 2/2] rfkill: use strict_strtoul

[Henrique de Moraes Holschuh]

On Sun, 17 Aug 2008, John W. Linville wrote:
> On Sat, Aug 02, 2008 at 02:56:26PM -0300, Henrique de Moraes Holschuh wrote:
> > Switch sysfs parsing to something that actually works properly.
> 
> > @@ -402,7 +410,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
> >  	if (error)
> >  		return error;
> >  
> > -	if (rfkill->user_claim != claim) {
> > +	if (!!rfkill->user_claim != !!claim) {
> >  		if (!claim) {
> >  			mutex_lock(&rfkill->mutex);
> >  			rfkill_toggle_radio(rfkill,
> 
> This looks a bit funny.  Is the '!!' in front of 'rfkill->user_claim'
> really necessary?  Especially since...

It is the safest way (read: it won't go wrong if we change user_claim and
claim types) to do it AFAIK, and the compiled is in charge of optimizing it
properly.

> > @@ -410,7 +418,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
> >  					    0);
> >  			mutex_unlock(&rfkill->mutex);
> >  		}
> > -		rfkill->user_claim = claim;
> > +		rfkill->user_claim = !!claim;
> >  	}
> >  
> >  	mutex_unlock(&rfkill_mutex);
> 
> You seem to be doing the only assignment to 'rfkill->user_claim',
> using a '!!' to condition the input?

user_claim is externally accessible, I thought it best to play safe.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh