Here is a small set of fixes for rfkill, mostly stuff I noticed while I
was working on other patches that I will send in for RFC and testing,
later.
It does include the documentation change requested by Ivo that makes
rfkill_force_state non-optional where applicable (devices that get
external state changes).
Shortlog:
Henrique de Moraes Holschuh (4):
rfkill: document rfkill_force_state as required
rfkill: fix led-trigger unregister order in error unwind
rfkill: document the rfkill struct locking
rfkill: mutex fixes
Please consider for merging for 2.6.26, as they are fixes and not
enhancements. Ivo, please be specially hard on the last patch, I did
test it here, but locking is always tricky...
Patches based on wireless-testing master.
--
"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
Every time a new input device that is capable of one of the rfkill EV_SW
events (currently only SW_RFKILL_ALL) is connected to rfkill-input, we must
check the switch state and take action.
Otherwise, we will ignore the initial switch state.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
---
Sorry, I forgot to send in this one as well. It is the last of the fixes
I have for 2.6.27, so far.
net/rfkill/rfkill-input.c | 45 ++++++++++++++++++++++++++++++---------------
1 files changed, 30 insertions(+), 15 deletions(-)
diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index 8aa8227..9203124 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -109,6 +109,25 @@ static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WWAN);
+static void rfkill_schedule_evsw_rfkillall(int state)
+{
+ /* EVERY radio type. data != 0 means radios ON */
+ /* handle EPO (emergency power off) through shortcut */
+ if (state) {
+ rfkill_schedule_set(&rfkill_wwan,
+ RFKILL_STATE_UNBLOCKED);
+ rfkill_schedule_set(&rfkill_wimax,
+ RFKILL_STATE_UNBLOCKED);
+ rfkill_schedule_set(&rfkill_uwb,
+ RFKILL_STATE_UNBLOCKED);
+ rfkill_schedule_set(&rfkill_bt,
+ RFKILL_STATE_UNBLOCKED);
+ rfkill_schedule_set(&rfkill_wlan,
+ RFKILL_STATE_UNBLOCKED);
+ } else
+ rfkill_schedule_epo();
+}
+
static void rfkill_event(struct input_handle *handle, unsigned int type,
unsigned int code, int data)
{
@@ -132,21 +151,7 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
} else if (type == EV_SW) {
switch (code) {
case SW_RFKILL_ALL:
- /* EVERY radio type. data != 0 means radios ON */
- /* handle EPO (emergency power off) through shortcut */
- if (data) {
- rfkill_schedule_set(&rfkill_wwan,
- RFKILL_STATE_UNBLOCKED);
- rfkill_schedule_set(&rfkill_wimax,
- RFKILL_STATE_UNBLOCKED);
- rfkill_schedule_set(&rfkill_uwb,
- RFKILL_STATE_UNBLOCKED);
- rfkill_schedule_set(&rfkill_bt,
- RFKILL_STATE_UNBLOCKED);
- rfkill_schedule_set(&rfkill_wlan,
- RFKILL_STATE_UNBLOCKED);
- } else
- rfkill_schedule_epo();
+ rfkill_schedule_evsw_rfkillall(data);
break;
default:
break;
@@ -154,6 +159,13 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
}
}
+static void rfkill_handle_evsw(const unsigned long *swbit,
+ const unsigned long *sw)
+{
+ if (test_bit(SW_RFKILL_ALL, swbit))
+ rfkill_schedule_evsw_rfkillall(test_bit(SW_RFKILL_ALL, sw));
+}
+
static int rfkill_connect(struct input_handler *handler, struct input_dev *dev,
const struct input_device_id *id)
{
@@ -176,6 +188,9 @@ static int rfkill_connect(struct input_handler *handler, struct input_dev *dev,
if (error)
goto err_unregister_handle;
+ if (test_bit(EV_SW, &dev->evbit))
+ rfkill_handle_evsw(&dev->swbit[0], &dev->sw[0]);
+
return 0;
err_unregister_handle:
--
1.5.6.2
On Saturday 19 July 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 19 Jul 2008, Ivo van Doorn wrote:
> > > @@ -150,7 +150,7 @@ static void update_rfkill_state(struct rfkill *rfkill)
> > > * even if the radio is in RFKILL_STATE_HARD_BLOCKED state, so as to
> > > * give the driver a hint that it should double-BLOCK the transmitter.
> > > *
> > > - * Caller must have aquired rfkill_mutex.
> > > + * Caller must have acquired rfkill->mutex.
> >
> > Should rfkill_toggle_radio() not grab the rfkill->mutex itself?
> > At the moment every caller to rfkill_toggle_radio() does:
> >
> > mutex_lock(&rfkill->mutex);
> > rfkill_toggle_radio(rfkill, state, 0);
> > mutex_unlock(&rfkill->mutex);
> >
> > without anything in between, so perhaps the safest way would be moving
> > the locking requirement into the function.
>
> sysfs attributes need to use mutex_lock_interruptible or
> mutex_lock_killable, and I also need it with external locking in some later
> patches that I haven't sent yet because I am reviewing them.
Ok, in that case it can be kept externally.
I believe there is a patch for sparse soon which adds __requires annotation
which we can use to make sparse check for the correct locking. ;)
> > > @@ -521,8 +527,11 @@ static void rfkill_remove_switch(struct rfkill *rfkill)
> > > {
> > > mutex_lock(&rfkill_mutex);
> > > list_del_init(&rfkill->node);
> > > - rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > > mutex_unlock(&rfkill_mutex);
> > > +
> > > + mutex_lock(&rfkill->mutex);
> > > + rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > > + mutex_unlock(&rfkill->mutex);
> >
> > Not sure about this one, something tells me it should be something like:
> >
> > mutex_lock(&rfkill_mutex);
> > list_del_init(&rfkill->node);
> >
> > mutex_lock(&rfkill->mutex);
> > rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > mutex_unlock(&rfkill->mutex);
> >
> > mutex_unlock(&rfkill_mutex);
>
> We really shouldn't need the nesting, as once we have deleted something from
> the list (which is always iterated and manipulated with rfkill_mutex
> locked), nothing that would need the rfkill_mutex will access that rfkill
> struct anymore.
>
> Nesting wouldn't hurt anything, though. If you really feel better with
> that nesting in place, I can nest them.
Ok, I now have looked long at this piece of code, and I think your version is
correct. No need to nest it.
Ivo
On Tuesday 15 July 2008, Henrique de Moraes Holschuh wrote:
> There are two mutexes in rfkill:
>
> rfkill->mutex, which protects some of the fields of a rfkill struct, and is
> also used for callback serialization.
>
> rfkill_mutex, which protects the global state, the list of registered
> rfkill structs and rfkill->claim.
>
> Make sure to use the correct mutex, and to not miss locking rfkill->mutex
> even when we already took rfkill_mutex.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> ---
> net/rfkill/rfkill.c | 29 +++++++++++++++++++----------
> 1 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index fc3a4fd..ac205ec 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -150,7 +150,7 @@ static void update_rfkill_state(struct rfkill *rfkill)
> * even if the radio is in RFKILL_STATE_HARD_BLOCKED state, so as to
> * give the driver a hint that it should double-BLOCK the transmitter.
> *
> - * Caller must have aquired rfkill_mutex.
> + * Caller must have acquired rfkill->mutex.
Should rfkill_toggle_radio() not grab the rfkill->mutex itself?
At the moment every caller to rfkill_toggle_radio() does:
mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill, state, 0);
mutex_unlock(&rfkill->mutex);
without anything in between, so perhaps the safest way would be moving
the locking requirement into the function.
> */
> static int rfkill_toggle_radio(struct rfkill *rfkill,
> enum rfkill_state state,
> @@ -216,8 +216,11 @@ void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
> rfkill_states[type] = state;
>
> list_for_each_entry(rfkill, &rfkill_list, node) {
> - if ((!rfkill->user_claim) && (rfkill->type == type))
> + if ((!rfkill->user_claim) && (rfkill->type == type)) {
> + mutex_lock(&rfkill->mutex);
> rfkill_toggle_radio(rfkill, state, 0);
> + mutex_unlock(&rfkill->mutex);
> + }
> }
>
> mutex_unlock(&rfkill_mutex);
> @@ -228,7 +231,7 @@ 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.
> + * everything in its path but rfkill_mutex and rfkill->mutex.
> */
> void rfkill_epo(void)
> {
> @@ -236,7 +239,9 @@ void rfkill_epo(void)
>
> mutex_lock(&rfkill_mutex);
> list_for_each_entry(rfkill, &rfkill_list, node) {
> + mutex_lock(&rfkill->mutex);
> rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> + mutex_unlock(&rfkill->mutex);
> }
> mutex_unlock(&rfkill_mutex);
> }
> @@ -372,6 +377,9 @@ static ssize_t rfkill_claim_store(struct device *dev,
> if (!capable(CAP_NET_ADMIN))
> return -EPERM;
>
> + if (rfkill->user_claim_unsupported)
> + return -EOPNOTSUPP;
> +
> /*
> * Take the global lock to make sure the kernel is not in
> * the middle of rfkill_switch_all
> @@ -380,19 +388,17 @@ static ssize_t rfkill_claim_store(struct device *dev,
> if (error)
> return error;
>
> - if (rfkill->user_claim_unsupported) {
> - error = -EOPNOTSUPP;
> - goto out_unlock;
> - }
> if (rfkill->user_claim != claim) {
> - if (!claim)
> + if (!claim) {
> + mutex_lock(&rfkill->mutex);
> rfkill_toggle_radio(rfkill,
> rfkill_states[rfkill->type],
> 0);
> + mutex_unlock(&rfkill->mutex);
> + }
> rfkill->user_claim = claim;
> }
>
> -out_unlock:
> mutex_unlock(&rfkill_mutex);
>
> return error ? error : count;
> @@ -521,8 +527,11 @@ static void rfkill_remove_switch(struct rfkill *rfkill)
> {
> mutex_lock(&rfkill_mutex);
> list_del_init(&rfkill->node);
> - rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> mutex_unlock(&rfkill_mutex);
> +
> + mutex_lock(&rfkill->mutex);
> + rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> + mutex_unlock(&rfkill->mutex);
Not sure about this one, something tells me it should be something like:
mutex_lock(&rfkill_mutex);
list_del_init(&rfkill->node);
mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
mutex_unlock(&rfkill->mutex);
mutex_unlock(&rfkill_mutex);
Ivo
On Wed, 16 Jul 2008, Dan Williams wrote:
> > Please consider for merging for 2.6.26, as they are fixes and not
> > enhancements. Ivo, please be specially hard on the last patch, I did
> > test it here, but locking is always tricky...
>
> If you do really mean 2.6.26, that's out already so you'd want to send
> the patches to stable@ as well. But I assume you mean 2.6.27, right?
Yeah, it is 2.6.27. Sorry about that. I don't think these patches are
needed in 2.6.26 at all, it is not like we have ever seen issues with rfkill
in the wild anyway, and it doesn't have very much widespread use yet.
I consider that promoting something to -stable is Ivo's call, really. But
if I ever happen to find a NULL derrefernce or something like that, I will
be sure to ask about it just as a head's up.
--
"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
There are two mutexes in rfkill:
rfkill->mutex, which protects some of the fields of a rfkill struct, and is
also used for callback serialization.
rfkill_mutex, which protects the global state, the list of registered
rfkill structs and rfkill->claim.
Make sure to use the correct mutex, and to not miss locking rfkill->mutex
even when we already took rfkill_mutex.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
net/rfkill/rfkill.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index fc3a4fd..ac205ec 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -150,7 +150,7 @@ static void update_rfkill_state(struct rfkill *rfkill)
* even if the radio is in RFKILL_STATE_HARD_BLOCKED state, so as to
* give the driver a hint that it should double-BLOCK the transmitter.
*
- * Caller must have aquired rfkill_mutex.
+ * Caller must have acquired rfkill->mutex.
*/
static int rfkill_toggle_radio(struct rfkill *rfkill,
enum rfkill_state state,
@@ -216,8 +216,11 @@ void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
rfkill_states[type] = state;
list_for_each_entry(rfkill, &rfkill_list, node) {
- if ((!rfkill->user_claim) && (rfkill->type == type))
+ if ((!rfkill->user_claim) && (rfkill->type == type)) {
+ mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill, state, 0);
+ mutex_unlock(&rfkill->mutex);
+ }
}
mutex_unlock(&rfkill_mutex);
@@ -228,7 +231,7 @@ 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.
+ * everything in its path but rfkill_mutex and rfkill->mutex.
*/
void rfkill_epo(void)
{
@@ -236,7 +239,9 @@ void rfkill_epo(void)
mutex_lock(&rfkill_mutex);
list_for_each_entry(rfkill, &rfkill_list, node) {
+ mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
+ mutex_unlock(&rfkill->mutex);
}
mutex_unlock(&rfkill_mutex);
}
@@ -372,6 +377,9 @@ static ssize_t rfkill_claim_store(struct device *dev,
if (!capable(CAP_NET_ADMIN))
return -EPERM;
+ if (rfkill->user_claim_unsupported)
+ return -EOPNOTSUPP;
+
/*
* Take the global lock to make sure the kernel is not in
* the middle of rfkill_switch_all
@@ -380,19 +388,17 @@ static ssize_t rfkill_claim_store(struct device *dev,
if (error)
return error;
- if (rfkill->user_claim_unsupported) {
- error = -EOPNOTSUPP;
- goto out_unlock;
- }
if (rfkill->user_claim != claim) {
- if (!claim)
+ if (!claim) {
+ mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill,
rfkill_states[rfkill->type],
0);
+ mutex_unlock(&rfkill->mutex);
+ }
rfkill->user_claim = claim;
}
-out_unlock:
mutex_unlock(&rfkill_mutex);
return error ? error : count;
@@ -521,8 +527,11 @@ static void rfkill_remove_switch(struct rfkill *rfkill)
{
mutex_lock(&rfkill_mutex);
list_del_init(&rfkill->node);
- rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
mutex_unlock(&rfkill_mutex);
+
+ mutex_lock(&rfkill->mutex);
+ rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
+ mutex_unlock(&rfkill->mutex);
}
/**
--
1.5.6.2
On Thu, Jul 17, 2008 at 6:56 AM, Michael Buesch <[email protected]> wrote:
> On Tuesday 15 July 2008, Henrique de Moraes Holschuh wrote:
>> There are two mutexes in rfkill:
>>
>> rfkill->mutex, which protects some of the fields of a rfkill struct, and is
>> also used for callback serialization.
>>
>> rfkill_mutex, which protects the global state, the list of registered
>> rfkill structs and rfkill->claim.
>>
>> Make sure to use the correct mutex, and to not miss locking rfkill->mutex
>> even when we already took rfkill_mutex.
rfkill_state_mutex ?
On Sat, 19 Jul 2008, Ivo van Doorn wrote:
> On Tuesday 15 July 2008, Henrique de Moraes Holschuh wrote:
> > Reorder fields in struct rfkill and add comments to make it clear
> > which fields are protected by rfkill->mutex.
> >
> > Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> > Cc: Ivo van Doorn <[email protected]>
> > ---
> > include/linux/rfkill.h | 7 ++++---
> > 1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> > index c5f6e54..630b573 100644
> > --- a/include/linux/rfkill.h
> > +++ b/include/linux/rfkill.h
> > @@ -89,13 +89,14 @@ struct rfkill {
> > const char *name;
> > enum rfkill_type type;
> >
> > - enum rfkill_state state;
> > + void *data;
> > bool user_claim_unsupported;
> > bool user_claim;
> >
> > + /* the mutex serializes callbacks and also protects
> > + * the state */
>
> Please move/copy the comment into the kerneldoc above the
> structure declaration as well.
Done.
> > struct mutex mutex;
> > -
> > - void *data;
>
> Not a real problem, but the data pointer doesn't need to be moved,
> since it is only used for the callback functions. So technically it is
> under the protection of the mutex as well.
Ok, I won't move it then, since it means any changes (not that anyone
should BE changing that field) would need to be under the mutex.
--
"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
On Sat, 19 Jul 2008, Ivo van Doorn wrote:
> > Every time the driver gets a notification from the card that one of its rfkill
> > lines changed state (polling might be needed on badly designed cards that don't
> > @@ -422,13 +423,24 @@ of the hardware is unknown), or read-write (where the hardware can be queried
> > about its current state).
> >
> > The rfkill class will call the get_state hook of a device every time it needs
> > -to know the *real* current state of the hardware. This can happen often.
> > +to know the *real* current state of the hardware. This can happen often, but
> > +it does not do any pooling, so it is not enough on hardware that is subject
> > +to state changes outside of the rfkill subsystem.
>
> pooling -> polling
>
> With that typo fixed, you can add my:
>
> Acked-by: Ivo van Doorn <[email protected]>
Done, will send v2 soon.
--
"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
> Every time the driver gets a notification from the card that one of its rfkill
> lines changed state (polling might be needed on badly designed cards that don't
> @@ -422,13 +423,24 @@ of the hardware is unknown), or read-write (where the hardware can be queried
> about its current state).
>
> The rfkill class will call the get_state hook of a device every time it needs
> -to know the *real* current state of the hardware. This can happen often.
> +to know the *real* current state of the hardware. This can happen often, but
> +it does not do any pooling, so it is not enough on hardware that is subject
> +to state changes outside of the rfkill subsystem.
pooling -> polling
With that typo fixed, you can add my:
Acked-by: Ivo van Doorn <[email protected]>
Ivo
On Tuesday 15 July 2008, Henrique de Moraes Holschuh wrote:
> Reorder fields in struct rfkill and add comments to make it clear
> which fields are protected by rfkill->mutex.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> ---
> include/linux/rfkill.h | 7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index c5f6e54..630b573 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -89,13 +89,14 @@ struct rfkill {
> const char *name;
> enum rfkill_type type;
>
> - enum rfkill_state state;
> + void *data;
> bool user_claim_unsupported;
> bool user_claim;
>
> + /* the mutex serializes callbacks and also protects
> + * the state */
Please move/copy the comment into the kerneldoc above the
structure declaration as well.
> struct mutex mutex;
> -
> - void *data;
Not a real problem, but the data pointer doesn't need to be moved,
since it is only used for the callback functions. So technically it is
under the protection of the mutex as well.
> + enum rfkill_state state;
> int (*toggle_radio)(void *data, enum rfkill_state state);
> int (*get_state)(void *data, enum rfkill_state *state);
Ivo
rfkill needs to unregister the led trigger AFTER a call to
rfkill_remove_switch(), otherwise it will not update the LED state,
possibly leaving it ON when it should be OFF.
To make led-trigger unregistering safer, guard against unregistering a
trigger twice, and also against issuing trigger events to a led trigger
that was unregistered. This makes the error unwind paths more resilient.
Refer to commit 8a8f1c0437a77cce29c1cb6089f01f22a6d9ca6e,
"rfkill: Register LED triggers before registering switch".
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Michael Buesch <[email protected]>
Cc: Dmitry Baryshkov <[email protected]>
---
net/rfkill/rfkill.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 022fe50..fc3a4fd 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -590,8 +590,10 @@ static void rfkill_led_trigger_register(struct rfkill *rfkill)
static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
{
#ifdef CONFIG_RFKILL_LEDS
- if (rfkill->led_trigger.name)
+ if (rfkill->led_trigger.name) {
led_trigger_unregister(&rfkill->led_trigger);
+ rfkill->led_trigger.name = NULL;
+ }
#endif
}
@@ -627,8 +629,8 @@ int rfkill_register(struct rfkill *rfkill)
error = device_add(dev);
if (error) {
- rfkill_led_trigger_unregister(rfkill);
rfkill_remove_switch(rfkill);
+ rfkill_led_trigger_unregister(rfkill);
return error;
}
--
1.5.6.2
On Tuesday 15 July 2008, Henrique de Moraes Holschuh wrote:
> There are two mutexes in rfkill:
>
> rfkill->mutex, which protects some of the fields of a rfkill struct, and is
> also used for callback serialization.
>
> rfkill_mutex, which protects the global state, the list of registered
> rfkill structs and rfkill->claim.
>
> Make sure to use the correct mutex, and to not miss locking rfkill->mutex
> even when we already took rfkill_mutex.
Can't we rename the global rfkill_mutex?
I think the current situation is highly confusing.
Perhaps rfkill_global_mutex would be good enough. Maybe somebody has a better
idea, however.
On Sat, 19 Jul 2008, Ivo van Doorn wrote:
> > @@ -150,7 +150,7 @@ static void update_rfkill_state(struct rfkill *rfkill)
> > * even if the radio is in RFKILL_STATE_HARD_BLOCKED state, so as to
> > * give the driver a hint that it should double-BLOCK the transmitter.
> > *
> > - * Caller must have aquired rfkill_mutex.
> > + * Caller must have acquired rfkill->mutex.
>
> Should rfkill_toggle_radio() not grab the rfkill->mutex itself?
> At the moment every caller to rfkill_toggle_radio() does:
>
> mutex_lock(&rfkill->mutex);
> rfkill_toggle_radio(rfkill, state, 0);
> mutex_unlock(&rfkill->mutex);
>
> without anything in between, so perhaps the safest way would be moving
> the locking requirement into the function.
sysfs attributes need to use mutex_lock_interruptible or
mutex_lock_killable, and I also need it with external locking in some later
patches that I haven't sent yet because I am reviewing them.
> > @@ -521,8 +527,11 @@ static void rfkill_remove_switch(struct rfkill *rfkill)
> > {
> > mutex_lock(&rfkill_mutex);
> > list_del_init(&rfkill->node);
> > - rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > mutex_unlock(&rfkill_mutex);
> > +
> > + mutex_lock(&rfkill->mutex);
> > + rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > + mutex_unlock(&rfkill->mutex);
>
> Not sure about this one, something tells me it should be something like:
>
> mutex_lock(&rfkill_mutex);
> list_del_init(&rfkill->node);
>
> mutex_lock(&rfkill->mutex);
> rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> mutex_unlock(&rfkill->mutex);
>
> mutex_unlock(&rfkill_mutex);
We really shouldn't need the nesting, as once we have deleted something from
the list (which is always iterated and manipulated with rfkill_mutex
locked), nothing that would need the rfkill_mutex will access that rfkill
struct anymore.
Nesting wouldn't hurt anything, though. If you really feel better with
that nesting in place, I can nest them.
--
"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
On Tue, 2008-07-15 at 16:32 -0300, Henrique de Moraes Holschuh wrote:
> Here is a small set of fixes for rfkill, mostly stuff I noticed while I
> was working on other patches that I will send in for RFC and testing,
> later.
>
> It does include the documentation change requested by Ivo that makes
> rfkill_force_state non-optional where applicable (devices that get
> external state changes).
>
> Shortlog:
>
> Henrique de Moraes Holschuh (4):
> rfkill: document rfkill_force_state as required
> rfkill: fix led-trigger unregister order in error unwind
> rfkill: document the rfkill struct locking
> rfkill: mutex fixes
>
> Please consider for merging for 2.6.26, as they are fixes and not
> enhancements. Ivo, please be specially hard on the last patch, I did
> test it here, but locking is always tricky...
If you do really mean 2.6.26, that's out already so you'd want to send
the patches to stable@ as well. But I assume you mean 2.6.27, right?
Dan
> Patches based on wireless-testing master.
>
On Thu, 17 Jul 2008, drago01 wrote:
> On Thu, Jul 17, 2008 at 6:56 AM, Michael Buesch <[email protected]> wrote:
> > On Tuesday 15 July 2008, Henrique de Moraes Holschuh wrote:
> >> There are two mutexes in rfkill:
> >>
> >> rfkill->mutex, which protects some of the fields of a rfkill struct, and is
> >> also used for callback serialization.
> >>
> >> rfkill_mutex, which protects the global state, the list of registered
> >> rfkill structs and rfkill->claim.
> >>
> >> Make sure to use the correct mutex, and to not miss locking rfkill->mutex
> >> even when we already took rfkill_mutex.
>
> rfkill_state_mutex ?
rfkill_mutex protects a lot of stuff, and does general serialization inside
rfkill.c. rfkill_global_mutex is a good name for it, yes.
rfkill->mutex protects more than the state, better leave that one alone. If
I find a way to do it without LONG periods under spinlock_irqsave, I'd make
it a spinlock to let drivers call some stuff in interrupt/atomic context...
--
"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
While the rfkill class does work with just get_state(), it doesn't work
well on devices that are subject to external events that cause rfkill state
changes.
Document that rfkill_force_state() is required in those cases.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
Documentation/rfkill.txt | 20 ++++++++++++++++----
net/rfkill/rfkill.c | 7 ++++++-
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index 0843ed0..1f924e4 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -390,9 +390,10 @@ rfkill lines are inactive, it must return RFKILL_STATE_SOFT_BLOCKED if its soft
rfkill input line is active. Only if none of the rfkill input lines are
active, will it return RFKILL_STATE_UNBLOCKED.
-If it doesn't implement the get_state() hook, it must make sure that its calls
-to rfkill_force_state() are enough to keep the status always up-to-date, and it
-must do a rfkill_force_state() on resume from sleep.
+Since the device has a hardware rfkill line, it IS subject to state changes
+external to rfkill. Therefore, the driver must make sure that it calls
+rfkill_force_state() to keep the status always up-to-date, and it must do a
+rfkill_force_state() on resume from sleep.
Every time the driver gets a notification from the card that one of its rfkill
lines changed state (polling might be needed on badly designed cards that don't
@@ -422,13 +423,24 @@ of the hardware is unknown), or read-write (where the hardware can be queried
about its current state).
The rfkill class will call the get_state hook of a device every time it needs
-to know the *real* current state of the hardware. This can happen often.
+to know the *real* current state of the hardware. This can happen often, but
+it does not do any pooling, so it is not enough on hardware that is subject
+to state changes outside of the rfkill subsystem.
+
+Therefore, calling rfkill_force_state() when a state change happens is
+mandatory when the device has a hardware rfkill line, or when something else
+like the firmware could cause its state to be changed without going through the
+rfkill class.
Some hardware provides events when its status changes. In these cases, it is
best for the driver to not provide a get_state hook, and instead register the
rfkill class *already* with the correct status, and keep it updated using
rfkill_force_state() when it gets an event from the hardware.
+rfkill_force_state() must be used on the device resume handlers to update the
+rfkill status, should there be any chance of the device status changing during
+the sleep.
+
There is no provision for a statically-allocated rfkill struct. You must
use rfkill_allocate() to allocate one.
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 7a560b7..022fe50 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -252,7 +252,12 @@ EXPORT_SYMBOL_GPL(rfkill_epo);
* a notification by the firmware/hardware of the current *real*
* state of the radio rfkill switch.
*
- * It may not be called from an atomic context.
+ * Devices which are subject to external changes on their rfkill
+ * state (such as those caused by a hardware rfkill line) MUST
+ * have their driver arrange to call rfkill_force_state() as soon
+ * as possible after such a change.
+ *
+ * This function may not be called from an atomic context.
*/
int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
{
--
1.5.6.2
On Tuesday 15 July 2008, Henrique de Moraes Holschuh wrote:
> rfkill needs to unregister the led trigger AFTER a call to
> rfkill_remove_switch(), otherwise it will not update the LED state,
> possibly leaving it ON when it should be OFF.
>
> To make led-trigger unregistering safer, guard against unregistering a
> trigger twice, and also against issuing trigger events to a led trigger
> that was unregistered. This makes the error unwind paths more resilient.
>
> Refer to commit 8a8f1c0437a77cce29c1cb6089f01f22a6d9ca6e,
> "rfkill: Register LED triggers before registering switch".
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> Cc: Michael Buesch <[email protected]>
> Cc: Dmitry Baryshkov <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> net/rfkill/rfkill.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 022fe50..fc3a4fd 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -590,8 +590,10 @@ static void rfkill_led_trigger_register(struct rfkill *rfkill)
> static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
> {
> #ifdef CONFIG_RFKILL_LEDS
> - if (rfkill->led_trigger.name)
> + if (rfkill->led_trigger.name) {
> led_trigger_unregister(&rfkill->led_trigger);
> + rfkill->led_trigger.name = NULL;
> + }
> #endif
> }
>
> @@ -627,8 +629,8 @@ int rfkill_register(struct rfkill *rfkill)
>
> error = device_add(dev);
> if (error) {
> - rfkill_led_trigger_unregister(rfkill);
> rfkill_remove_switch(rfkill);
> + rfkill_led_trigger_unregister(rfkill);
> return error;
> }
>
Hi Henrique,
On Thu, Jul 17, 2008 at 06:56:54PM -0300, Henrique de Moraes Holschuh wrote:
> Every time a new input device that is capable of one of the rfkill EV_SW
> events (currently only SW_RFKILL_ALL) is connected to rfkill-input, we must
> check the switch state and take action.
>
> Otherwise, we will ignore the initial switch state.
>
I think this task is more suited for the ->start() method of the input
handler. This way, if userspace grabs the switch input device and then
releases it rfkill will get a chance to re-synchronize with the switch
state.
Also I think it would be a good idea to take input_dev->event_lock
while scheduling the state change so that the switch state does not
change underneath you.
--
Dmitry
On Saturday 19 July 2008, Ivo van Doorn wrote:
> On Saturday 19 July 2008, Henrique de Moraes Holschuh wrote:
> > On Sat, 19 Jul 2008, Ivo van Doorn wrote:
> > > > @@ -150,7 +150,7 @@ static void update_rfkill_state(struct rfkill *rfkill)
> > > > * even if the radio is in RFKILL_STATE_HARD_BLOCKED state, so as to
> > > > * give the driver a hint that it should double-BLOCK the transmitter.
> > > > *
> > > > - * Caller must have aquired rfkill_mutex.
> > > > + * Caller must have acquired rfkill->mutex.
> > >
> > > Should rfkill_toggle_radio() not grab the rfkill->mutex itself?
> > > At the moment every caller to rfkill_toggle_radio() does:
> > >
> > > mutex_lock(&rfkill->mutex);
> > > rfkill_toggle_radio(rfkill, state, 0);
> > > mutex_unlock(&rfkill->mutex);
> > >
> > > without anything in between, so perhaps the safest way would be moving
> > > the locking requirement into the function.
> >
> > sysfs attributes need to use mutex_lock_interruptible or
> > mutex_lock_killable, and I also need it with external locking in some later
> > patches that I haven't sent yet because I am reviewing them.
>
> Ok, in that case it can be kept externally.
>
> I believe there is a patch for sparse soon which adds __requires annotation
> which we can use to make sparse check for the correct locking. ;)
>
> > > > @@ -521,8 +527,11 @@ static void rfkill_remove_switch(struct rfkill *rfkill)
> > > > {
> > > > mutex_lock(&rfkill_mutex);
> > > > list_del_init(&rfkill->node);
> > > > - rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > > > mutex_unlock(&rfkill_mutex);
> > > > +
> > > > + mutex_lock(&rfkill->mutex);
> > > > + rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > > > + mutex_unlock(&rfkill->mutex);
> > >
> > > Not sure about this one, something tells me it should be something like:
> > >
> > > mutex_lock(&rfkill_mutex);
> > > list_del_init(&rfkill->node);
> > >
> > > mutex_lock(&rfkill->mutex);
> > > rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> > > mutex_unlock(&rfkill->mutex);
> > >
> > > mutex_unlock(&rfkill_mutex);
> >
> > We really shouldn't need the nesting, as once we have deleted something from
> > the list (which is always iterated and manipulated with rfkill_mutex
> > locked), nothing that would need the rfkill_mutex will access that rfkill
> > struct anymore.
> >
> > Nesting wouldn't hurt anything, though. If you really feel better with
> > that nesting in place, I can nest them.
>
> Ok, I now have looked long at this piece of code, and I think your version is
> correct. No need to nest it.
P.S. That makes it an:
Acked-by: Ivo van Doorn <[email protected]>
Reorder fields in struct rfkill and add comments to make it clear
which fields are protected by rfkill->mutex.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
include/linux/rfkill.h | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index c5f6e54..630b573 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -89,13 +89,14 @@ struct rfkill {
const char *name;
enum rfkill_type type;
- enum rfkill_state state;
+ void *data;
bool user_claim_unsupported;
bool user_claim;
+ /* the mutex serializes callbacks and also protects
+ * the state */
struct mutex mutex;
-
- void *data;
+ enum rfkill_state state;
int (*toggle_radio)(void *data, enum rfkill_state state);
int (*get_state)(void *data, enum rfkill_state *state);
--
1.5.6.2