[GIT PATCH] rfkill fixes

[GIT PATCH] rfkill fixes

[Henrique de Moraes Holschuh]

John,

Here are two fixes (one bug fix, the other is just minor kernel-doc
stuff) for the rfkill subsystem in wireless-testing and -next.

The patches are also available at:
git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git for-upstream/linux-wireless

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: some minor kernel-doc changes for rfkill_toggle_radio

[Henrique de Moraes Holschuh]

Improve rfkill_toggle_radio's kernel-doc header a bit.

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

diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index ce0e231..aa7039d 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -130,17 +130,19 @@ static void update_rfkill_state(struct rfkill *rfkill)
 
 /**
  * rfkill_toggle_radio - wrapper for toggle_radio hook
- * calls toggle_radio taking into account a lot of "small"
- * details.
+ *
  * @rfkill: the rfkill struct to use
  * @force: calls toggle_radio even if cache says it is not needed,
  *	and also makes sure notifications of the state will be
  *	sent even if it didn't change
  * @state: the new state to call toggle_radio() with
  *
- * This wrappen protects and enforces the API for toggle_radio
- * calls.  Note that @force cannot override a (possibly cached)
- * state of RFKILL_STATE_HARD_BLOCKED.  Any device making use of
+ * Calls rfkill->toggle_radio, enforcing the API for toggle_radio
+ * calls and handling all the red tape such as issuing notifications
+ * if the call is successful.
+ *
+ * Note that @force 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
  * rfkill_force_state(), so the cache either is bypassed or valid.
  *
-- 
1.5.5.4

[PATCH 2/2] rfkill: ignore errors from rfkill_toggle_radio in rfkill_add_switch

[Henrique de Moraes Holschuh]

rfkill_add_switch() calls rfkill_toggle_radio() to set the state of a
recently registered rfkill class to the current global state [for that
rfkill->type].

The rfkill_toggle_radio() call is going to error out if the hardware is
RFKILL_STATE_HARD_BLOCKED, and the global state is RFKILL_STATE_UNBLOCKED.

That is a quite normal situation which I missed to account for.  As things
stand, the error return from rfkill_toggle_radio ends up causing
rfkill_register to bail out with an error (de-registering the new switch in
the process), which is Not Nice.

Change rfkill_add_switch() to not return errors because of a failed call to
rfkill_toggle_radio().  We can go back to returning errors again (if that's
indeed the right thing to do) if we define the exact error codes the
rfkill->toggle_radio callbacks are to return in each situation, so that we
can ignore the right ones only.

Bug reported by "kionez <kionez@anche.no>".

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

diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index aa7039d..7a560b7 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -501,17 +501,15 @@ static struct class rfkill_class = {
 
 static int rfkill_add_switch(struct rfkill *rfkill)
 {
-	int error;
-
 	mutex_lock(&rfkill_mutex);
 
-	error = rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
-	if (!error)
-		list_add_tail(&rfkill->node, &rfkill_list);
+	rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
+
+	list_add_tail(&rfkill->node, &rfkill_list);
 
 	mutex_unlock(&rfkill_mutex);
 
-	return error;
+	return 0;
 }
 
 static void rfkill_remove_switch(struct rfkill *rfkill)
-- 
1.5.5.4

Re: [PATCH 1/2] rfkill: some minor kernel-doc changes for rfkill_toggle_radio

[Ivo van Doorn]

On Thursday 03 July 2008, Henrique de Moraes Holschuh wrote:
> Improve rfkill_toggle_radio's kernel-doc header a bit.
> 
> 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 |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index ce0e231..aa7039d 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -130,17 +130,19 @@ static void update_rfkill_state(struct rfkill *rfkill)
>  
>  /**
>   * rfkill_toggle_radio - wrapper for toggle_radio hook
> - * calls toggle_radio taking into account a lot of "small"
> - * details.
> + *
>   * @rfkill: the rfkill struct to use
>   * @force: calls toggle_radio even if cache says it is not needed,
>   *	and also makes sure notifications of the state will be
>   *	sent even if it didn't change
>   * @state: the new state to call toggle_radio() with
>   *
> - * This wrappen protects and enforces the API for toggle_radio
> - * calls.  Note that @force cannot override a (possibly cached)
> - * state of RFKILL_STATE_HARD_BLOCKED.  Any device making use of
> + * Calls rfkill->toggle_radio, enforcing the API for toggle_radio
> + * calls and handling all the red tape such as issuing notifications
> + * if the call is successful.
> + *
> + * Note that @force 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
>   * rfkill_force_state(), so the cache either is bypassed or valid.
>   *

Re: [PATCH 2/2] rfkill: ignore errors from rfkill_toggle_radio in rfkill_add_switch

[Ivo van Doorn]

On Thursday 03 July 2008, Henrique de Moraes Holschuh wrote:
> rfkill_add_switch() calls rfkill_toggle_radio() to set the state of a
> recently registered rfkill class to the current global state [for that
> rfkill->type].
> 
> The rfkill_toggle_radio() call is going to error out if the hardware is
> RFKILL_STATE_HARD_BLOCKED, and the global state is RFKILL_STATE_UNBLOCKED.
> 
> That is a quite normal situation which I missed to account for.  As things
> stand, the error return from rfkill_toggle_radio ends up causing
> rfkill_register to bail out with an error (de-registering the new switch in
> the process), which is Not Nice.
> 
> Change rfkill_add_switch() to not return errors because of a failed call to
> rfkill_toggle_radio().  We can go back to returning errors again (if that's
> indeed the right thing to do) if we define the exact error codes the
> rfkill->toggle_radio callbacks are to return in each situation, so that we
> can ignore the right ones only.
> 
> Bug reported by "kionez <kionez@anche.no>".
> 
> 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>

> Cc: kionez <kionez@anche.no>
> ---
>  net/rfkill/rfkill.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index aa7039d..7a560b7 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -501,17 +501,15 @@ static struct class rfkill_class = {
>  
>  static int rfkill_add_switch(struct rfkill *rfkill)
>  {
> -	int error;
> -
>  	mutex_lock(&rfkill_mutex);
>  
> -	error = rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
> -	if (!error)
> -		list_add_tail(&rfkill->node, &rfkill_list);
> +	rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
> +
> +	list_add_tail(&rfkill->node, &rfkill_list);
>  
>  	mutex_unlock(&rfkill_mutex);
>  
> -	return error;
> +	return 0;
>  }
>  
>  static void rfkill_remove_switch(struct rfkill *rfkill)

Re: [PATCH 2/2] rfkill: ignore errors from rfkill_toggle_radio in rfkill_add_switch

[Tomas Winkler]

On Thu, Jul 3, 2008 at 7:14 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> rfkill_add_switch() calls rfkill_toggle_radio() to set the state of a
> recently registered rfkill class to the current global state [for that
> rfkill->type].
>
> The rfkill_toggle_radio() call is going to error out if the hardware is
> RFKILL_STATE_HARD_BLOCKED, and the global state is RFKILL_STATE_UNBLOCKED.
>
> That is a quite normal situation which I missed to account for.  As things
> stand, the error return from rfkill_toggle_radio ends up causing
> rfkill_register to bail out with an error (de-registering the new switch in
> the process), which is Not Nice.
>
> Change rfkill_add_switch() to not return errors because of a failed call to
> rfkill_toggle_radio().  We can go back to returning errors again (if that's
> indeed the right thing to do) if we define the exact error codes the
> rfkill->toggle_radio callbacks are to return in each situation, so that we
> can ignore the right ones only.
>
> Bug reported by "kionez <kionez@anche.no>".
>
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: kionez <kionez@anche.no>
> ---
>  net/rfkill/rfkill.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index aa7039d..7a560b7 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -501,17 +501,15 @@ static struct class rfkill_class = {
>
>  static int rfkill_add_switch(struct rfkill *rfkill)
>  {
> -       int error;
> -
>        mutex_lock(&rfkill_mutex);
>
> -       error = rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
> -       if (!error)
> -               list_add_tail(&rfkill->node, &rfkill_list);
> +       rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
> +
> +       list_add_tail(&rfkill->node, &rfkill_list);
>
>        mutex_unlock(&rfkill_mutex);
>
> -       return error;
> +       return 0;
>  }

So why this is not a void function
Thanks
Tomas
>  static void rfkill_remove_switch(struct rfkill *rfkill)
> --
> 1.5.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/2] rfkill: ignore errors from rfkill_toggle_radio in rfkill_add_switch

[Tomas Winkler]

On Thu, Jul 3, 2008 at 7:47 PM, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> On Thursday 03 July 2008, Tomas Winkler wrote:
>> On Thu, Jul 3, 2008 at 7:14 PM, Henrique de Moraes Holschuh
>> <hmh@hmh.eng.br> wrote:
>> > rfkill_add_switch() calls rfkill_toggle_radio() to set the state of a
>> > recently registered rfkill class to the current global state [for that
>> > rfkill->type].
>> >
>> > The rfkill_toggle_radio() call is going to error out if the hardware is
>> > RFKILL_STATE_HARD_BLOCKED, and the global state is RFKILL_STATE_UNBLOCKED.
>> >
>> > That is a quite normal situation which I missed to account for.  As things
>> > stand, the error return from rfkill_toggle_radio ends up causing
>> > rfkill_register to bail out with an error (de-registering the new switch in
>> > the process), which is Not Nice.
>> >
>> > Change rfkill_add_switch() to not return errors because of a failed call to
>> > rfkill_toggle_radio().  We can go back to returning errors again (if that's
>> > indeed the right thing to do) if we define the exact error codes the
>> > rfkill->toggle_radio callbacks are to return in each situation, so that we
>> > can ignore the right ones only.
>> >
>> > Bug reported by "kionez <kionez@anche.no>".
>> >
>> > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>> > Cc: Ivo van Doorn <IvDoorn@gmail.com>
>> > Cc: kionez <kionez@anche.no>
>> > ---
>> >  net/rfkill/rfkill.c |   10 ++++------
>> >  1 files changed, 4 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
>> > index aa7039d..7a560b7 100644
>> > --- a/net/rfkill/rfkill.c
>> > +++ b/net/rfkill/rfkill.c
>> > @@ -501,17 +501,15 @@ static struct class rfkill_class = {
>> >
>> >  static int rfkill_add_switch(struct rfkill *rfkill)
>> >  {
>> > -       int error;
>> > -
>> >        mutex_lock(&rfkill_mutex);
>> >
>> > -       error = rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
>> > -       if (!error)
>> > -               list_add_tail(&rfkill->node, &rfkill_list);
>> > +       rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
>> > +
>> > +       list_add_tail(&rfkill->node, &rfkill_list);
>> >
>> >        mutex_unlock(&rfkill_mutex);
>> >
>> > -       return error;
>> > +       return 0;
>> >  }
>>
>> So why this is not a void function
>
> Well as Henrique suggested we might add a correct return value later when
> we figured out which error codes should be returned. If the interface now goes
> to a void function, we need to fix all drivers only later to revert those changes
> again when it returns an int again.
> And this way drivers will at least keep it mind that the function might fail,
> perhaps not now, but perhaps later, when for example we are sane-checking
> the filled in fields of the rfkill structure, or do some other fancy things which
> might fail.

Fair enough
Tomas

Re: [PATCH 2/2] rfkill: ignore errors from rfkill_toggle_radio in rfkill_add_switch

[Ivo van Doorn]

On Thursday 03 July 2008, Tomas Winkler wrote:
> On Thu, Jul 3, 2008 at 7:14 PM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> > rfkill_add_switch() calls rfkill_toggle_radio() to set the state of a
> > recently registered rfkill class to the current global state [for that
> > rfkill->type].
> >
> > The rfkill_toggle_radio() call is going to error out if the hardware is
> > RFKILL_STATE_HARD_BLOCKED, and the global state is RFKILL_STATE_UNBLOCKED.
> >
> > That is a quite normal situation which I missed to account for.  As things
> > stand, the error return from rfkill_toggle_radio ends up causing
> > rfkill_register to bail out with an error (de-registering the new switch in
> > the process), which is Not Nice.
> >
> > Change rfkill_add_switch() to not return errors because of a failed call to
> > rfkill_toggle_radio().  We can go back to returning errors again (if that's
> > indeed the right thing to do) if we define the exact error codes the
> > rfkill->toggle_radio callbacks are to return in each situation, so that we
> > can ignore the right ones only.
> >
> > Bug reported by "kionez <kionez@anche.no>".
> >
> > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > Cc: Ivo van Doorn <IvDoorn@gmail.com>
> > Cc: kionez <kionez@anche.no>
> > ---
> >  net/rfkill/rfkill.c |   10 ++++------
> >  1 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> > index aa7039d..7a560b7 100644
> > --- a/net/rfkill/rfkill.c
> > +++ b/net/rfkill/rfkill.c
> > @@ -501,17 +501,15 @@ static struct class rfkill_class = {
> >
> >  static int rfkill_add_switch(struct rfkill *rfkill)
> >  {
> > -       int error;
> > -
> >        mutex_lock(&rfkill_mutex);
> >
> > -       error = rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
> > -       if (!error)
> > -               list_add_tail(&rfkill->node, &rfkill_list);
> > +       rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
> > +
> > +       list_add_tail(&rfkill->node, &rfkill_list);
> >
> >        mutex_unlock(&rfkill_mutex);
> >
> > -       return error;
> > +       return 0;
> >  }
> 
> So why this is not a void function

Well as Henrique suggested we might add a correct return value later when
we figured out which error codes should be returned. If the interface now goes
to a void function, we need to fix all drivers only later to revert those changes
again when it returns an int again.
And this way drivers will at least keep it mind that the function might fail,
perhaps not now, but perhaps later, when for example we are sane-checking
the filled in fields of the rfkill structure, or do some other fancy things which
might fail.

Ivo