[PATCH v3] rfkill: Add rfkill-any LED trigger

[PATCH v3] rfkill: Add rfkill-any LED trigger

[Michał Kępień]

Add a new "global" (i.e. not per-rfkill device) LED trigger, rfkill-any,
which may be useful on laptops with a single "radio LED" and multiple
radio transmitters.  The trigger is meant to turn a LED on whenever
there is at least one radio transmitter active and turn it off
otherwise.

This requires taking rfkill_global_mutex before calling
rfkill_set_block() in rfkill_resume(): since
rfkill_any_led_trigger_event(true) is called from rfkill_set_block()
unconditionally, each caller of the latter needs to take care of locking
rfkill_global_mutex.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
Jonathan, I refrained from resending patch 1/2 from v2 as part of this
series as it is currently applied in mac80211-next/master along with
Arnd's fix.  Please let me know if you would like me to handle this
differently.

Mike, could you please test whether this version works fine on your
machine?  Thanks!

Changes from v2:

  - Handle the global mutex properly when rfkill_set_{hw,sw}_state() or
    rfkill_set_states() is called from within an rfkill callback.  v2
    always tried to lock the global mutex in such a case, which led to a
    deadlock when an rfkill driver called one of the above functions
    from its query or set_block callback.  This is solved by defining a
    new bitfield, RFKILL_BLOCK_SW_HASLOCK, which is set before the above
    callbacks are invoked and cleared afterwards; the functions listed
    above use this bitfield to tell rfkill_any_led_trigger_event()
    whether the global mutex is currently held or not.
    RFKILL_BLOCK_SW_SETCALL cannot be reused for this purpose as setting
    it before invoking the query callback would cause any calls to
    rfkill_set_sw_state() made from within that callback to work on
    RFKILL_BLOCK_SW_PREV instead of RFKILL_BLOCK_SW and thus change the
    way rfkill_set_block() behaves.

  - As rfkill_any_led_trigger_event() now takes a boolean argument which
    tells it whether the global mutex was already taken by the caller,
    all calls to __rfkill_any_led_trigger_event() outside
    rfkill_any_led_trigger_event() have been replaced with calls to
    rfkill_any_led_trigger_event(true).

 net/rfkill/core.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 86 insertions(+), 4 deletions(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index afa4f71b4c7b..688eac7b97ef 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -44,6 +44,7 @@
 #define RFKILL_BLOCK_ANY	(RFKILL_BLOCK_HW |\
 				 RFKILL_BLOCK_SW |\
 				 RFKILL_BLOCK_SW_PREV)
+#define RFKILL_BLOCK_SW_HASLOCK	BIT(30)
 #define RFKILL_BLOCK_SW_SETCALL	BIT(31)
 
 struct rfkill {
@@ -176,6 +177,51 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
 {
 	led_trigger_unregister(&rfkill->led_trigger);
 }
+
+static struct led_trigger rfkill_any_led_trigger;
+
+static void __rfkill_any_led_trigger_event(void)
+{
+	enum led_brightness brightness = LED_OFF;
+	struct rfkill *rfkill;
+
+	list_for_each_entry(rfkill, &rfkill_list, node) {
+		if (!(rfkill->state & RFKILL_BLOCK_ANY)) {
+			brightness = LED_FULL;
+			break;
+		}
+	}
+
+	led_trigger_event(&rfkill_any_led_trigger, brightness);
+}
+
+static void rfkill_any_led_trigger_event(bool global_locked)
+{
+	if (global_locked) {
+		__rfkill_any_led_trigger_event();
+	} else {
+		mutex_lock(&rfkill_global_mutex);
+		__rfkill_any_led_trigger_event();
+		mutex_unlock(&rfkill_global_mutex);
+	}
+}
+
+static void rfkill_any_led_trigger_activate(struct led_classdev *led_cdev)
+{
+	rfkill_any_led_trigger_event(false);
+}
+
+static int rfkill_any_led_trigger_register(void)
+{
+	rfkill_any_led_trigger.name = "rfkill-any";
+	rfkill_any_led_trigger.activate = rfkill_any_led_trigger_activate;
+	return led_trigger_register(&rfkill_any_led_trigger);
+}
+
+static void rfkill_any_led_trigger_unregister(void)
+{
+	led_trigger_unregister(&rfkill_any_led_trigger);
+}
 #else
 static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
@@ -189,6 +235,19 @@ static inline int rfkill_led_trigger_register(struct rfkill *rfkill)
 static inline void rfkill_led_trigger_unregister(struct rfkill *rfkill)
 {
 }
+
+static void rfkill_any_led_trigger_event(bool global_locked)
+{
+}
+
+static int rfkill_any_led_trigger_register(void)
+{
+	return 0;
+}
+
+static void rfkill_any_led_trigger_unregister(void)
+{
+}
 #endif /* CONFIG_RFKILL_LEDS */
 
 static void rfkill_fill_event(struct rfkill_event *ev, struct rfkill *rfkill,
@@ -253,6 +312,10 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 	if (unlikely(rfkill->dev.power.power_state.event & PM_EVENT_SLEEP))
 		return;
 
+	spin_lock_irqsave(&rfkill->lock, flags);
+	rfkill->state |= RFKILL_BLOCK_SW_HASLOCK;
+	spin_unlock_irqrestore(&rfkill->lock, flags);
+
 	/*
 	 * Some platforms (...!) generate input events which affect the
 	 * _hard_ kill state -- whenever something tries to change the
@@ -292,11 +355,13 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 			rfkill->state &= ~RFKILL_BLOCK_SW;
 	}
 	rfkill->state &= ~RFKILL_BLOCK_SW_SETCALL;
+	rfkill->state &= ~RFKILL_BLOCK_SW_HASLOCK;
 	rfkill->state &= ~RFKILL_BLOCK_SW_PREV;
 	curr = rfkill->state & RFKILL_BLOCK_SW;
 	spin_unlock_irqrestore(&rfkill->lock, flags);
 
 	rfkill_led_trigger_event(rfkill);
+	rfkill_any_led_trigger_event(true);
 
 	if (prev != curr)
 		rfkill_event(rfkill);
@@ -463,7 +528,7 @@ bool rfkill_get_global_sw_state(const enum rfkill_type type)
 bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked)
 {
 	unsigned long flags;
-	bool ret, prev;
+	bool ret, prev, global_locked;
 
 	BUG_ON(!rfkill);
 
@@ -474,9 +539,11 @@ bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked)
 	else
 		rfkill->state &= ~RFKILL_BLOCK_HW;
 	ret = !!(rfkill->state & RFKILL_BLOCK_ANY);
+	global_locked = !!(rfkill->state & RFKILL_BLOCK_SW_HASLOCK);
 	spin_unlock_irqrestore(&rfkill->lock, flags);
 
 	rfkill_led_trigger_event(rfkill);
+	rfkill_any_led_trigger_event(global_locked);
 
 	if (rfkill->registered && prev != blocked)
 		schedule_work(&rfkill->uevent_work);
@@ -502,7 +569,7 @@ static void __rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
 bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
 {
 	unsigned long flags;
-	bool prev, hwblock;
+	bool prev, hwblock, global_locked;
 
 	BUG_ON(!rfkill);
 
@@ -511,6 +578,7 @@ bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
 	__rfkill_set_sw_state(rfkill, blocked);
 	hwblock = !!(rfkill->state & RFKILL_BLOCK_HW);
 	blocked = blocked || hwblock;
+	global_locked = !!(rfkill->state & RFKILL_BLOCK_SW_HASLOCK);
 	spin_unlock_irqrestore(&rfkill->lock, flags);
 
 	if (!rfkill->registered)
@@ -520,6 +588,7 @@ bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
 		schedule_work(&rfkill->uevent_work);
 
 	rfkill_led_trigger_event(rfkill);
+	rfkill_any_led_trigger_event(global_locked);
 
 	return blocked;
 }
@@ -542,7 +611,7 @@ EXPORT_SYMBOL(rfkill_init_sw_state);
 void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
 {
 	unsigned long flags;
-	bool swprev, hwprev;
+	bool swprev, hwprev, global_locked;
 
 	BUG_ON(!rfkill);
 
@@ -559,6 +628,7 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
 		rfkill->state |= RFKILL_BLOCK_HW;
 	else
 		rfkill->state &= ~RFKILL_BLOCK_HW;
+	global_locked = !!(rfkill->state & RFKILL_BLOCK_SW_HASLOCK);
 
 	spin_unlock_irqrestore(&rfkill->lock, flags);
 
@@ -569,6 +639,7 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
 			schedule_work(&rfkill->uevent_work);
 
 		rfkill_led_trigger_event(rfkill);
+		rfkill_any_led_trigger_event(global_locked);
 	}
 }
 EXPORT_SYMBOL(rfkill_set_states);
@@ -812,8 +883,10 @@ static int rfkill_resume(struct device *dev)
 	rfkill->suspended = false;
 
 	if (!rfkill->persistent) {
+		mutex_lock(&rfkill_global_mutex);
 		cur = !!(rfkill->state & RFKILL_BLOCK_SW);
 		rfkill_set_block(rfkill, cur);
+		mutex_unlock(&rfkill_global_mutex);
 	}
 
 	if (rfkill->ops->poll && !rfkill->polling_paused)
@@ -985,6 +1058,7 @@ int __must_check rfkill_register(struct rfkill *rfkill)
 #endif
 	}
 
+	rfkill_any_led_trigger_event(true);
 	rfkill_send_events(rfkill, RFKILL_OP_ADD);
 
 	mutex_unlock(&rfkill_global_mutex);
@@ -1017,6 +1091,7 @@ void rfkill_unregister(struct rfkill *rfkill)
 	mutex_lock(&rfkill_global_mutex);
 	rfkill_send_events(rfkill, RFKILL_OP_DEL);
 	list_del_init(&rfkill->node);
+	rfkill_any_led_trigger_event(true);
 	mutex_unlock(&rfkill_global_mutex);
 
 	rfkill_led_trigger_unregister(rfkill);
@@ -1269,6 +1344,10 @@ static int __init rfkill_init(void)
 	if (error)
 		goto error_misc;
 
+	error = rfkill_any_led_trigger_register();
+	if (error)
+		goto error_led_trigger;
+
 #ifdef CONFIG_RFKILL_INPUT
 	error = rfkill_handler_init();
 	if (error)
@@ -1279,8 +1358,10 @@ static int __init rfkill_init(void)
 
 #ifdef CONFIG_RFKILL_INPUT
 error_input:
-	misc_deregister(&rfkill_miscdev);
+	rfkill_any_led_trigger_unregister();
 #endif
+error_led_trigger:
+	misc_deregister(&rfkill_miscdev);
 error_misc:
 	class_unregister(&rfkill_class);
 error_class:
@@ -1293,6 +1374,7 @@ static void __exit rfkill_exit(void)
 #ifdef CONFIG_RFKILL_INPUT
 	rfkill_handler_exit();
 #endif
+	rfkill_any_led_trigger_unregister();
 	misc_deregister(&rfkill_miscdev);
 	class_unregister(&rfkill_class);
 }
-- 
2.11.0

Re: [PATCH v3] rfkill: Add rfkill-any LED trigger

[Johannes Berg]

>   - Handle the global mutex properly when rfkill_set_{hw,sw}_state()
> or
>     rfkill_set_states() is called from within an rfkill callback.  v2
>     always tried to lock the global mutex in such a case, which led
> to a
>     deadlock when an rfkill driver called one of the above functions
>     from its query or set_block callback.  This is solved by defining
> a
>     new bitfield, RFKILL_BLOCK_SW_HASLOCK, which is set before the
> above
>     callbacks are invoked and cleared afterwards; the functions
> listed
>     above use this bitfield to tell rfkill_any_led_trigger_event()
>     whether the global mutex is currently held or not.
>     RFKILL_BLOCK_SW_SETCALL cannot be reused for this purpose as
> setting
>     it before invoking the query callback would cause any calls to
>     rfkill_set_sw_state() made from within that callback to work on
>     RFKILL_BLOCK_SW_PREV instead of RFKILL_BLOCK_SW and thus change
> the
>     way rfkill_set_block() behaves.

I'm not super happy with this conditional locking - can't we instead
defer the necessary work to a workqueue, or so, for purposes of the
LED?

johannes

[PATCH v3] rfkill: Add rfkill-any LED trigger

[Mike Krinkin]

On Wed, Dec 21, 2016 at 09:45:33AM +0100, Michał Kępień wrote:
> Add a new "global" (i.e. not per-rfkill device) LED trigger, rfkill-any,
> which may be useful on laptops with a single "radio LED" and multiple
> radio transmitters.  The trigger is meant to turn a LED on whenever
> there is at least one radio transmitter active and turn it off
> otherwise.
> 
> This requires taking rfkill_global_mutex before calling
> rfkill_set_block() in rfkill_resume(): since
> rfkill_any_led_trigger_event(true) is called from rfkill_set_block()
> unconditionally, each caller of the latter needs to take care of locking
> rfkill_global_mutex.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
> Jonathan, I refrained from resending patch 1/2 from v2 as part of this
> series as it is currently applied in mac80211-next/master along with
> Arnd's fix.  Please let me know if you would like me to handle this
> differently.
> 
> Mike, could you please test whether this version works fine on your
> machine?  Thanks!

Sorry for the delay, patch works fine for me.

> 
> Changes from v2:
> 
>   - Handle the global mutex properly when rfkill_set_{hw,sw}_state() or
>     rfkill_set_states() is called from within an rfkill callback.  v2
>     always tried to lock the global mutex in such a case, which led to a
>     deadlock when an rfkill driver called one of the above functions
>     from its query or set_block callback.  This is solved by defining a
>     new bitfield, RFKILL_BLOCK_SW_HASLOCK, which is set before the above
>     callbacks are invoked and cleared afterwards; the functions listed
>     above use this bitfield to tell rfkill_any_led_trigger_event()
>     whether the global mutex is currently held or not.
>     RFKILL_BLOCK_SW_SETCALL cannot be reused for this purpose as setting
>     it before invoking the query callback would cause any calls to
>     rfkill_set_sw_state() made from within that callback to work on
>     RFKILL_BLOCK_SW_PREV instead of RFKILL_BLOCK_SW and thus change the
>     way rfkill_set_block() behaves.
> 
>   - As rfkill_any_led_trigger_event() now takes a boolean argument which
>     tells it whether the global mutex was already taken by the caller,
>     all calls to __rfkill_any_led_trigger_event() outside
>     rfkill_any_led_trigger_event() have been replaced with calls to
>     rfkill_any_led_trigger_event(true).
> 
>  net/rfkill/core.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index afa4f71b4c7b..688eac7b97ef 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -44,6 +44,7 @@
>  #define RFKILL_BLOCK_ANY	(RFKILL_BLOCK_HW |\
>  				 RFKILL_BLOCK_SW |\
>  				 RFKILL_BLOCK_SW_PREV)
> +#define RFKILL_BLOCK_SW_HASLOCK	BIT(30)
>  #define RFKILL_BLOCK_SW_SETCALL	BIT(31)
>  
>  struct rfkill {
> @@ -176,6 +177,51 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
>  {
>  	led_trigger_unregister(&rfkill->led_trigger);
>  }
> +
> +static struct led_trigger rfkill_any_led_trigger;
> +
> +static void __rfkill_any_led_trigger_event(void)
> +{
> +	enum led_brightness brightness = LED_OFF;
> +	struct rfkill *rfkill;
> +
> +	list_for_each_entry(rfkill, &rfkill_list, node) {
> +		if (!(rfkill->state & RFKILL_BLOCK_ANY)) {
> +			brightness = LED_FULL;
> +			break;
> +		}
> +	}
> +
> +	led_trigger_event(&rfkill_any_led_trigger, brightness);
> +}
> +
> +static void rfkill_any_led_trigger_event(bool global_locked)
> +{
> +	if (global_locked) {
> +		__rfkill_any_led_trigger_event();
> +	} else {
> +		mutex_lock(&rfkill_global_mutex);
> +		__rfkill_any_led_trigger_event();
> +		mutex_unlock(&rfkill_global_mutex);
> +	}
> +}
> +
> +static void rfkill_any_led_trigger_activate(struct led_classdev *led_cdev)
> +{
> +	rfkill_any_led_trigger_event(false);
> +}
> +
> +static int rfkill_any_led_trigger_register(void)
> +{
> +	rfkill_any_led_trigger.name = "rfkill-any";
> +	rfkill_any_led_trigger.activate = rfkill_any_led_trigger_activate;
> +	return led_trigger_register(&rfkill_any_led_trigger);
> +}
> +
> +static void rfkill_any_led_trigger_unregister(void)
> +{
> +	led_trigger_unregister(&rfkill_any_led_trigger);
> +}
>  #else
>  static void rfkill_led_trigger_event(struct rfkill *rfkill)
>  {
> @@ -189,6 +235,19 @@ static inline int rfkill_led_trigger_register(struct rfkill *rfkill)
>  static inline void rfkill_led_trigger_unregister(struct rfkill *rfkill)
>  {
>  }
> +
> +static void rfkill_any_led_trigger_event(bool global_locked)
> +{
> +}
> +
> +static int rfkill_any_led_trigger_register(void)
> +{
> +	return 0;
> +}
> +
> +static void rfkill_any_led_trigger_unregister(void)
> +{
> +}
>  #endif /* CONFIG_RFKILL_LEDS */
>  
>  static void rfkill_fill_event(struct rfkill_event *ev, struct rfkill *rfkill,
> @@ -253,6 +312,10 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
>  	if (unlikely(rfkill->dev.power.power_state.event & PM_EVENT_SLEEP))
>  		return;
>  
> +	spin_lock_irqsave(&rfkill->lock, flags);
> +	rfkill->state |= RFKILL_BLOCK_SW_HASLOCK;
> +	spin_unlock_irqrestore(&rfkill->lock, flags);
> +
>  	/*
>  	 * Some platforms (...!) generate input events which affect the
>  	 * _hard_ kill state -- whenever something tries to change the
> @@ -292,11 +355,13 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
>  			rfkill->state &= ~RFKILL_BLOCK_SW;
>  	}
>  	rfkill->state &= ~RFKILL_BLOCK_SW_SETCALL;
> +	rfkill->state &= ~RFKILL_BLOCK_SW_HASLOCK;
>  	rfkill->state &= ~RFKILL_BLOCK_SW_PREV;
>  	curr = rfkill->state & RFKILL_BLOCK_SW;
>  	spin_unlock_irqrestore(&rfkill->lock, flags);
>  
>  	rfkill_led_trigger_event(rfkill);
> +	rfkill_any_led_trigger_event(true);
>  
>  	if (prev != curr)
>  		rfkill_event(rfkill);
> @@ -463,7 +528,7 @@ bool rfkill_get_global_sw_state(const enum rfkill_type type)
>  bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked)
>  {
>  	unsigned long flags;
> -	bool ret, prev;
> +	bool ret, prev, global_locked;
>  
>  	BUG_ON(!rfkill);
>  
> @@ -474,9 +539,11 @@ bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked)
>  	else
>  		rfkill->state &= ~RFKILL_BLOCK_HW;
>  	ret = !!(rfkill->state & RFKILL_BLOCK_ANY);
> +	global_locked = !!(rfkill->state & RFKILL_BLOCK_SW_HASLOCK);
>  	spin_unlock_irqrestore(&rfkill->lock, flags);
>  
>  	rfkill_led_trigger_event(rfkill);
> +	rfkill_any_led_trigger_event(global_locked);
>  
>  	if (rfkill->registered && prev != blocked)
>  		schedule_work(&rfkill->uevent_work);
> @@ -502,7 +569,7 @@ static void __rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
>  bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
>  {
>  	unsigned long flags;
> -	bool prev, hwblock;
> +	bool prev, hwblock, global_locked;
>  
>  	BUG_ON(!rfkill);
>  
> @@ -511,6 +578,7 @@ bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
>  	__rfkill_set_sw_state(rfkill, blocked);
>  	hwblock = !!(rfkill->state & RFKILL_BLOCK_HW);
>  	blocked = blocked || hwblock;
> +	global_locked = !!(rfkill->state & RFKILL_BLOCK_SW_HASLOCK);
>  	spin_unlock_irqrestore(&rfkill->lock, flags);
>  
>  	if (!rfkill->registered)
> @@ -520,6 +588,7 @@ bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
>  		schedule_work(&rfkill->uevent_work);
>  
>  	rfkill_led_trigger_event(rfkill);
> +	rfkill_any_led_trigger_event(global_locked);
>  
>  	return blocked;
>  }
> @@ -542,7 +611,7 @@ EXPORT_SYMBOL(rfkill_init_sw_state);
>  void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
>  {
>  	unsigned long flags;
> -	bool swprev, hwprev;
> +	bool swprev, hwprev, global_locked;
>  
>  	BUG_ON(!rfkill);
>  
> @@ -559,6 +628,7 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
>  		rfkill->state |= RFKILL_BLOCK_HW;
>  	else
>  		rfkill->state &= ~RFKILL_BLOCK_HW;
> +	global_locked = !!(rfkill->state & RFKILL_BLOCK_SW_HASLOCK);
>  
>  	spin_unlock_irqrestore(&rfkill->lock, flags);
>  
> @@ -569,6 +639,7 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
>  			schedule_work(&rfkill->uevent_work);
>  
>  		rfkill_led_trigger_event(rfkill);
> +		rfkill_any_led_trigger_event(global_locked);
>  	}
>  }
>  EXPORT_SYMBOL(rfkill_set_states);
> @@ -812,8 +883,10 @@ static int rfkill_resume(struct device *dev)
>  	rfkill->suspended = false;
>  
>  	if (!rfkill->persistent) {
> +		mutex_lock(&rfkill_global_mutex);
>  		cur = !!(rfkill->state & RFKILL_BLOCK_SW);
>  		rfkill_set_block(rfkill, cur);
> +		mutex_unlock(&rfkill_global_mutex);
>  	}
>  
>  	if (rfkill->ops->poll && !rfkill->polling_paused)
> @@ -985,6 +1058,7 @@ int __must_check rfkill_register(struct rfkill *rfkill)
>  #endif
>  	}
>  
> +	rfkill_any_led_trigger_event(true);
>  	rfkill_send_events(rfkill, RFKILL_OP_ADD);
>  
>  	mutex_unlock(&rfkill_global_mutex);
> @@ -1017,6 +1091,7 @@ void rfkill_unregister(struct rfkill *rfkill)
>  	mutex_lock(&rfkill_global_mutex);
>  	rfkill_send_events(rfkill, RFKILL_OP_DEL);
>  	list_del_init(&rfkill->node);
> +	rfkill_any_led_trigger_event(true);
>  	mutex_unlock(&rfkill_global_mutex);
>  
>  	rfkill_led_trigger_unregister(rfkill);
> @@ -1269,6 +1344,10 @@ static int __init rfkill_init(void)
>  	if (error)
>  		goto error_misc;
>  
> +	error = rfkill_any_led_trigger_register();
> +	if (error)
> +		goto error_led_trigger;
> +
>  #ifdef CONFIG_RFKILL_INPUT
>  	error = rfkill_handler_init();
>  	if (error)
> @@ -1279,8 +1358,10 @@ static int __init rfkill_init(void)
>  
>  #ifdef CONFIG_RFKILL_INPUT
>  error_input:
> -	misc_deregister(&rfkill_miscdev);
> +	rfkill_any_led_trigger_unregister();
>  #endif
> +error_led_trigger:
> +	misc_deregister(&rfkill_miscdev);
>  error_misc:
>  	class_unregister(&rfkill_class);
>  error_class:
> @@ -1293,6 +1374,7 @@ static void __exit rfkill_exit(void)
>  #ifdef CONFIG_RFKILL_INPUT
>  	rfkill_handler_exit();
>  #endif
> +	rfkill_any_led_trigger_unregister();
>  	misc_deregister(&rfkill_miscdev);
>  	class_unregister(&rfkill_class);
>  }
> -- 
> 2.11.0
> 

Re: [PATCH v3] rfkill: Add rfkill-any LED trigger

[Johannes Berg]

> I'm not super happy with this conditional locking - can't we instead
> defer the necessary work to a workqueue, or so, for purposes of the
> LED?

Actually, since you can sleep in here, and do various other things like
scheduling etc. this can't even be correct as is - one thread might be
in the probe and another might also attempt to do some operations that
require the lock but now don't take it.

johannes

Re: [PATCH v3] rfkill: Add rfkill-any LED trigger

[Johannes Berg]

On Mon, 2017-01-02 at 13:21 +0100, Johannes Berg wrote:
> > I'm not super happy with this conditional locking - can't we
> > instead
> > defer the necessary work to a workqueue, or so, for purposes of the
> > LED?
> 
> Actually, since you can sleep in here, and do various other things
> like scheduling etc. this can't even be correct as is - one thread
> might be in the probe and another might also attempt to do some
> operations that require the lock but now don't take it.

Additionally, this doesn't address the "can be called in any context"
part, only the "even from within rfkill callbacks" part. It's clearly
still not safe to call this from any context that is not allowed to
sleep, for example.

johannes

Re: [PATCH v3] rfkill: Add rfkill-any LED trigger

[Michał Kępień]

> On Mon, 2017-01-02 at 13:21 +0100, Johannes Berg wrote:
> > > I'm not super happy with this conditional locking - can't we
> > > instead
> > > defer the necessary work to a workqueue, or so, for purposes of the
> > > LED?
> > 
> > Actually, since you can sleep in here, and do various other things
> > like scheduling etc. this can't even be correct as is - one thread
> > might be in the probe and another might also attempt to do some
> > operations that require the lock but now don't take it.
> 
> Additionally, this doesn't address the "can be called in any context"
> part, only the "even from within rfkill callbacks" part. It's clearly
> still not safe to call this from any context that is not allowed to
> sleep, for example.

Thanks for reviewing.  I will attempt a workqueue-based approach in v4.

-- 
Best regards,
Michał Kępień