bluetooth: core: RfC - add basic LED triggers

PATCH Superseded Heiner Kallweit 2 participants 3 messages Dec 31, 2015 – Jan 1, 2016

Subsystems (2)

Bluetooth drivers Bluetooth

lore

Share & tools

lore

Heiner Kallweit

author

2015-12-31 18:29 UTC #0 lore raw

PATCH 1/1 bluetooth: core: RfC - add basic LED triggers

In the bluetooth subsystem I miss the option to use LEDs to indicate the status. This patch is based on the LED triggers in mac80211 and adds two basic LED triggers (powered up, device connected) plus a config option to enable the BT LED triggers.

It works fine on my system however I don't know the bluetooth stack in too much detail and therefore appreciate any comment.

Signed-off-by Heiner Kallweit <[email protected]>

---
 include/net/bluetooth/hci_core.h |   7 +++
 net/bluetooth/Kconfig            |   9 +++
 net/bluetooth/Makefile           |   1 +
 net/bluetooth/hci_core.c         |   8 +++
 net/bluetooth/hci_event.c        |   3 +
 net/bluetooth/led.c              | 126 +++++++++++++++++++++++++++++++++++++++
 net/bluetooth/led.h              |  32 ++++++++++
 7 files changed, 186 insertions(+)
 create mode 100644 net/bluetooth/led.c
 create mode 100644 net/bluetooth/led.h

include/net/bluetooth/hci_core.h +7 -0

index c95e032..e47f05c 100644
@@ -25,6 +25,7 @@
 #ifndef __HCI_CORE_H
 #define __HCI_CORE_H
 
+#include <linux/leds.h>
 #include <net/bluetooth/hci.h>
 #include <net/bluetooth/hci_sock.h>

@@ -395,6 +396,12 @@ struct hci_dev {
 	struct delayed_work	rpa_expired;
 	bdaddr_t		rpa;
 
+#ifdef CONFIG_BT_LEDS
+	struct led_trigger assoc_led, radio_led;
+	atomic_t assoc_led_active, radio_led_active;
+	atomic_t assoc_led_cnt, radio_led_cnt;
+#endif
+
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);

net/bluetooth/Kconfig +9 -0

index 95d1a66..cba56c7 100644
@@ -69,6 +69,15 @@ config BT_6LOWPAN
 	help
 	  IPv6 compression over Bluetooth Low Energy.
 
+config BT_LEDS
+	bool "Enable LED triggers"
+	depends on BT
+	depends on LEDS_CLASS
+	select LEDS_TRIGGERS
+	help
+	 This option selects a few LED triggers for different
+	 Bluetooth events.
+
 config BT_SELFTEST
 	bool "Bluetooth self testing support"
 	depends on BT && DEBUG_KERNEL

net/bluetooth/Makefile +1 -0

index 2b15ae8..98d8988 100644
@@ -17,6 +17,7 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
 
 bluetooth-$(CONFIG_BT_BREDR) += sco.o
 bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
+bluetooth-$(CONFIG_BT_LEDS) += led.o
 bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
 bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o

net/bluetooth/hci_core.c +8 -0

index 47bcef7..63a4229 100644
@@ -40,6 +40,7 @@
 #include "hci_request.h"
 #include "hci_debugfs.h"
 #include "smp.h"
+#include "led.h"
 
 static void hci_rx_work(struct work_struct *work);
 static void hci_cmd_work(struct work_struct *work);

@@ -1395,6 +1396,7 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 		hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
 		set_bit(HCI_UP, &hdev->flags);
 		hci_sock_dev_event(hdev, HCI_DEV_UP);
+		hci_led_radio(hdev, true);
 		if (!hci_dev_test_flag(hdev, HCI_SETUP) &&
 		    !hci_dev_test_flag(hdev, HCI_CONFIG) &&
 		    !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&

@@ -1532,6 +1534,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
 		return 0;
 	}
 
+	hci_led_radio(hdev, false);
+
 	/* Flush RX and TX works */
 	flush_work(&hdev->tx_work);
 	flush_work(&hdev->rx_work);

@@ -3067,6 +3071,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	if (error < 0)
 		goto err_wqueue;
 
+	hci_led_init(hdev);
+
 	hdev->rfkill = rfkill_alloc(hdev->name, &hdev->dev,
 				    RFKILL_TYPE_BLUETOOTH, &hci_rfkill_ops,
 				    hdev);

@@ -3128,6 +3134,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 
 	id = hdev->id;
 
+	hci_led_exit(hdev);
+
 	write_lock(&hci_dev_list_lock);
 	list_del(&hdev->list);
 	write_unlock(&hci_dev_list_lock);

net/bluetooth/hci_event.c +3 -0

index 7554da5..e5545c3 100644
@@ -35,6 +35,7 @@
 #include "a2mp.h"
 #include "amp.h"
 #include "smp.h"
+#include "led.h"
 
 #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
 		 "\x00\x00\x00\x00\x00\x00\x00\x00"

@@ -5239,6 +5240,7 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
 	case HCI_EV_CONN_COMPLETE:
 		hci_conn_complete_evt(hdev, skb);
+		hci_led_assoc(hdev, true);
 		break;
 
 	case HCI_EV_CONN_REQUEST:

@@ -5246,6 +5248,7 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		break;
 
 	case HCI_EV_DISCONN_COMPLETE:
+		hci_led_assoc(hdev, false);
 		hci_disconn_complete_evt(hdev, skb);
 		break;

net/bluetooth/led.c new file +126 -0

126 lines not on this page · raw message

net/bluetooth/led.h new file +32 -0

new file mode 100644
index 0000000..a607292
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2015, Heiner Kallweit <[email protected]>
+ * This code is based on the MAC80211 LED code.
+ * Original author: Johannes Berg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifdef CONFIG_BT_LEDS
+void hci_led_assoc(struct hci_dev *hdev, bool associated);
+void hci_led_radio(struct hci_dev *hdev, bool enabled);
+void hci_led_init(struct hci_dev *hdev);
+void hci_led_exit(struct hci_dev *hdev);
+#else
+static inline void hci_led_assoc(struct hci_dev *hdev, bool associated)
+{
+}
+
+static inline void hci_led_radio(struct hci_dev *hdev, bool enabled)
+{
+}
+
+static inline void hci_led_init(struct hci_dev *hdev)
+{
+}
+
+static inline void hci_led_exit(struct hci_dev *hdev)
+{
+}
+#endif

Signature · 1 line

2.6.4

Marcel Holtmann

maintainer

2015-12-31 18:54 UTC #1 lore raw

Hi Heiner,

Quoted 34 lines

In the bluetooth subsystem I miss the option to use LEDs to indicate the status. This patch is based on the LED triggers in mac80211 and adds two basic LED triggers (powered up, device connected) plus a config option to enable the BT LED triggers.
It works fine on my system however I don't know the bluetooth stack in too much detail and therefore appreciate any comment.
Signed-off-by Heiner Kallweit <[email protected]>
---
include/net/bluetooth/hci_core.h |   7 +++
net/bluetooth/Kconfig            |   9 +++
net/bluetooth/Makefile           |   1 +
net/bluetooth/hci_core.c         |   8 +++
net/bluetooth/hci_event.c        |   3 +
net/bluetooth/led.c              | 126 +++++++++++++++++++++++++++++++++++++++
net/bluetooth/led.h              |  32 ++++++++++
7 files changed, 186 insertions(+)
create mode 100644 net/bluetooth/led.c
create mode 100644 net/bluetooth/led.h
include/net/bluetooth/hci_core.h +1 -0
index c95e032..e47f05c 100644
@@ -25,6 +25,7 @@
 #ifndef __HCI_CORE_H
 #define __HCI_CORE_H
 
+#include <linux/leds.h>
 #include <net/bluetooth/hci.h>
 #include <net/bluetooth/hci_sock.h>
 

include/net/bluetooth/hci_core.h struct hci_dev { · line 404

Quoted 9 lines

include/net/bluetooth/hci_core.h +6 -0

@@ -395,6 +396,12 @@ struct hci_dev {
 	struct delayed_work	rpa_expired;
 	bdaddr_t		rpa;
 
+#ifdef CONFIG_BT_LEDS
+	struct led_trigger assoc_led, radio_led;
+	atomic_t assoc_led_active, radio_led_active;
+	atomic_t assoc_led_cnt, radio_led_cnt;
+#endif
+

not a big fan of these kind of assignments. Keep them separate on each line.

Also we might just want to start using a sub-struct and move that all to leds.[ch]. I wonder if we can do some magic compiler trick of avoid the ifdef here.

Quoted 9 lines

include/net/bluetooth/hci_core.h +0 -0

 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);

net/bluetooth/Kconfig +0 -0

index 95d1a66..cba56c7 100644
@@ -69,6 +69,15 @@ config BT_6LOWPAN
 	help

net/bluetooth/Kconfig config BT_6LOWPAN · line 79

Quoted 10 lines

net/bluetooth/Kconfig +8 -0

 	  IPv6 compression over Bluetooth Low Energy.
 
+config BT_LEDS
+	bool "Enable LED triggers"
+	depends on BT
+	depends on LEDS_CLASS
+	select LEDS_TRIGGERS
+	help
+	 This option selects a few LED triggers for different
+	 Bluetooth events.

Not sure if the diff is messing this up, but just in case, two spaces extra indentation for the help text body.

Quoted 10 lines

net/bluetooth/Kconfig +1 -0

config BT_SELFTEST

	bool "Bluetooth self testing support"
	depends on BT && DEBUG_KERNEL

net/bluetooth/Makefile +0 -0

index 2b15ae8..98d8988 100644
@@ -17,6 +17,7 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \

Quoted 3 lines

bluetooth-$(CONFIG_BT_BREDR) += sco.o
bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
+bluetooth-$(CONFIG_BT_LEDS) += led.o

I would use plural leds.c here.

Quoted 71 lines

bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o

net/bluetooth/hci_core.c +8 -0

index 47bcef7..63a4229 100644
@@ -40,6 +40,7 @@
 #include "hci_request.h"
 #include "hci_debugfs.h"
 #include "smp.h"
+#include "led.h"
 
 static void hci_rx_work(struct work_struct *work);
 static void hci_cmd_work(struct work_struct *work);

@@ -1395,6 +1396,7 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 		hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
 		set_bit(HCI_UP, &hdev->flags);
 		hci_sock_dev_event(hdev, HCI_DEV_UP);
+		hci_led_radio(hdev, true);
 		if (!hci_dev_test_flag(hdev, HCI_SETUP) &&
 		    !hci_dev_test_flag(hdev, HCI_CONFIG) &&
 		    !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&

@@ -1532,6 +1534,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
 		return 0;
 	}
 
+	hci_led_radio(hdev, false);
+
 	/* Flush RX and TX works */
 	flush_work(&hdev->tx_work);
 	flush_work(&hdev->rx_work);

@@ -3067,6 +3071,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	if (error < 0)
 		goto err_wqueue;
 
+	hci_led_init(hdev);
+
 	hdev->rfkill = rfkill_alloc(hdev->name, &hdev->dev,
 				    RFKILL_TYPE_BLUETOOTH, &hci_rfkill_ops,
 				    hdev);

@@ -3128,6 +3134,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 
 	id = hdev->id;
 
+	hci_led_exit(hdev);
+
 	write_lock(&hci_dev_list_lock);
 	list_del(&hdev->list);
 	write_unlock(&hci_dev_list_lock);

net/bluetooth/hci_event.c +2 -0

index 7554da5..e5545c3 100644
@@ -35,6 +35,7 @@
 #include "a2mp.h"
 #include "amp.h"
 #include "smp.h"
+#include "led.h"
 
 #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
 		 "\x00\x00\x00\x00\x00\x00\x00\x00"

@@ -5239,6 +5240,7 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
 	case HCI_EV_CONN_COMPLETE:
 		hci_conn_complete_evt(hdev, skb);
+		hci_led_assoc(hdev, true);
 		break;
 
 	case HCI_EV_CONN_REQUEST:

net/bluetooth/hci_event.c void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb) · line 5253

Quoted 6 lines

net/bluetooth/hci_event.c +1 -0

@@ -5246,6 +5248,7 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		break;
 
 	case HCI_EV_DISCONN_COMPLETE:
+		hci_led_assoc(hdev, false);
 		hci_disconn_complete_evt(hdev, skb);
 		break;

This is actually a really bad idea. It will only account for BR/EDR and in case of LE it will only catch the disconnect event. Which will lead to the WARN_ON with your atomic counter. Not to mention that also for SCO/eSCO links you get the same imbalance as well.

You need to hook this up into hci_conn handling that actually represents logical connections.

I think this makes also pretty clear that assoc is a pretty bad word in the Bluetooth world. It has no meaning for BR/EDR or LE operation. You want a connection state LED.

Now the question becomes if you actually want a connection state LED or an actual activity LED. For BR/EDR you have the controller be discovering devices or being actually in a connection or both. For LE you have discovery as well (active scanning) and being in a connection state. However you have additionally background auto-connection (passive scanning) and also advertising.

Thinking this further through, you could actually be a lot more detailed in the LE world. Here you have scanning state, advertising state and connection state (maybe also initiating state). With the concept of multiple instances of the state machine, you could have a separate LED for each of these.

The radio on/off trigger is simple and I most likely would just start with that one, but for the "assoc" trigger this needs a bit more thinking since Bluetooth does work different than WiFi.

Quoted 61 lines

net/bluetooth/led.c new file +54 -0

new file mode 100644
index 0000000..420633c
@@ -0,0 +1,126 @@
+/*
+ * Copyright 2015, Heiner Kallweit <[email protected]>
+ * This code is based on the MAC80211 LED code.
+ * Original author: Johannes Berg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "led.h"
+
+#define cdev_to_hdev(cdev, name) container_of(cdev->trigger, \
+					      struct hci_dev, \
+					      name)
+
+void hci_led_assoc(struct hci_dev *hdev, bool associated)
+{
+	int cnt;
+
+	if (associated)
+		cnt = atomic_inc_return(&hdev->assoc_led_cnt);
+	else
+		cnt = atomic_dec_return(&hdev->assoc_led_cnt);
+
+	WARN_ON(cnt < 0);
+
+	if (!atomic_read(&hdev->assoc_led_active))
+		return;
+
+	led_trigger_event(&hdev->assoc_led,
+			  cnt ? LED_FULL : LED_OFF);
+}
+
+void hci_led_radio(struct hci_dev *hdev, bool enabled)
+{
+	int cnt;
+
+	if (enabled)
+		cnt = atomic_inc_return(&hdev->radio_led_cnt);
+	else
+		cnt = atomic_dec_return(&hdev->radio_led_cnt);
+
+	WARN_ON(cnt < 0);
+
+	if (!atomic_read(&hdev->radio_led_active))
+		return;
+
+	led_trigger_event(&hdev->radio_led,
+			  cnt ? LED_FULL : LED_OFF);
+}

net/bluetooth/led.c line 64

Quoted 10 lines

net/bluetooth/led.c new file +10 -0

+
+static void hci_assoc_led_activate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, assoc_led);
+	int cnt = atomic_read(&hdev->assoc_led_cnt);
+
+	atomic_inc(&hdev->assoc_led_active);
+
+	led_trigger_event(&hdev->assoc_led, cnt ? LED_FULL : LED_OFF);
+}

Why is this so complicated with the LED active handling? Isn't that something the LED class should actually take care of for us. What is the point in repeating that over and over again. Actually all of this should most likely have some sort of "simple cumulative LED" version that does that.

Quoted 43 lines

net/bluetooth/led.c new file +43 -0

+
+static void hci_assoc_led_deactivate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, assoc_led);
+
+	atomic_dec(&hdev->assoc_led_active);
+}
+
+static void hci_radio_led_activate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, radio_led);
+	int cnt = atomic_read(&hdev->radio_led_cnt);
+
+	atomic_inc(&hdev->radio_led_active);
+
+	led_trigger_event(&hdev->radio_led, cnt ? LED_FULL : LED_OFF);
+}
+
+static void hci_radio_led_deactivate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, radio_led);
+
+	atomic_dec(&hdev->radio_led_active);
+}
+
+void hci_led_init(struct hci_dev *hdev)
+{
+	/* initialize assoc_led */
+	atomic_set(&hdev->assoc_led_cnt, 0);
+	atomic_set(&hdev->assoc_led_active, 0);
+	hdev->assoc_led.activate = hci_assoc_led_activate;
+	hdev->assoc_led.deactivate = hci_assoc_led_deactivate;
+	hdev->assoc_led.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+					      "%s-assoc", hdev->name);
+	if (hdev->assoc_led.name &&
+	    led_trigger_register(&hdev->assoc_led)) {
+		devm_kfree(&hdev->dev, (void *)hdev->assoc_led.name);
+		hdev->assoc_led.name = NULL;
+	}
+
+	/* initialize radio_led */
+	atomic_set(&hdev->radio_led_cnt, 0);
+	atomic_set(&hdev->radio_led_active, 0);

net/bluetooth/led.c line 117

Quoted 10 lines

net/bluetooth/led.c new file +10 -0

+	hdev->radio_led.activate = hci_radio_led_activate;
+	hdev->radio_led.deactivate = hci_radio_led_deactivate;
+	hdev->radio_led.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+					      "%s-radio", hdev->name);
+	if (hdev->radio_led.name &&
+	    led_trigger_register(&hdev->radio_led)) {
+		devm_kfree(&hdev->dev, (void *)hdev->radio_led.name);
+		hdev->radio_led.name = NULL;
+	}
+}

Now also the real question is if you just want a per controller LEDs or also some global LED that combines that status of all controllers attached to the system.

Quoted 37 lines

net/bluetooth/led.c new file +9 -0

+
+void hci_led_exit(struct hci_dev *hdev)
+{
+	if (hdev->assoc_led.name)
+		led_trigger_unregister(&hdev->assoc_led);
+	if (hdev->radio_led.name)
+		led_trigger_unregister(&hdev->radio_led);
+}
+

net/bluetooth/led.h new file +22 -0

new file mode 100644
index 0000000..a607292
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2015, Heiner Kallweit <[email protected]>
+ * This code is based on the MAC80211 LED code.
+ * Original author: Johannes Berg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifdef CONFIG_BT_LEDS
+void hci_led_assoc(struct hci_dev *hdev, bool associated);
+void hci_led_radio(struct hci_dev *hdev, bool enabled);
+void hci_led_init(struct hci_dev *hdev);
+void hci_led_exit(struct hci_dev *hdev);
+#else
+static inline void hci_led_assoc(struct hci_dev *hdev, bool associated)
+{
+}
+
+static inline void hci_led_radio(struct hci_dev *hdev, bool enabled)
+{

net/bluetooth/led.h line 32

Quoted 10 lines

net/bluetooth/led.h new file +10 -0

+}
+
+static inline void hci_led_init(struct hci_dev *hdev)
+{
+}
+
+static inline void hci_led_exit(struct hci_dev *hdev)
+{
+}
+#endif

Regards

Marcel

Heiner Kallweit

author

2016-01-01 15:56 UTC #2 lore raw

Am 31.12.2015 um 19:54 schrieb Marcel Holtmann:

Quoted 48 lines

Hi Heiner,
Quoted 34 lines
In the bluetooth subsystem I miss the option to use LEDs to indicate the status. This patch is based on the LED triggers in mac80211 and adds two basic LED triggers (powered up, device connected) plus a config option to enable the BT LED triggers.
It works fine on my system however I don't know the bluetooth stack in too much detail and therefore appreciate any comment.
Signed-off-by Heiner Kallweit <[email protected]>
---
include/net/bluetooth/hci_core.h |   7 +++
net/bluetooth/Kconfig            |   9 +++
net/bluetooth/Makefile           |   1 +
net/bluetooth/hci_core.c         |   8 +++
net/bluetooth/hci_event.c        |   3 +
net/bluetooth/led.c              | 126 +++++++++++++++++++++++++++++++++++++++
net/bluetooth/led.h              |  32 ++++++++++
7 files changed, 186 insertions(+)
create mode 100644 net/bluetooth/led.c
create mode 100644 net/bluetooth/led.h
include/net/bluetooth/hci_core.h +1 -0
index c95e032..e47f05c 100644
@@ -25,6 +25,7 @@
 #ifndef __HCI_CORE_H
 #define __HCI_CORE_H
 
+#include <linux/leds.h>
 #include <net/bluetooth/hci.h>
 #include <net/bluetooth/hci_sock.h>
 
include/net/bluetooth/hci_core.h struct hci_dev { · line 404
Quoted 9 lines
include/net/bluetooth/hci_core.h +6 -0
@@ -395,6 +396,12 @@ struct hci_dev {
 	struct delayed_work	rpa_expired;
 	bdaddr_t		rpa;
 
+#ifdef CONFIG_BT_LEDS
+	struct led_trigger assoc_led, radio_led;
+	atomic_t assoc_led_active, radio_led_active;
+	atomic_t assoc_led_cnt, radio_led_cnt;
+#endif
+
not a big fan of these kind of assignments. Keep them separate on each line.

Quoted 2 lines

Also we might just want to start using a sub-struct and move that all to leds.[ch]. I wonder if we can do some magic compiler trick of avoid the ifdef here.

OK. Avoiding the ifdef should be possible considering that struct led_trigger is also defined (as an empty struct) if CONFIG_LEDS_TRIGGERS is not set.

Quoted 11 lines

include/net/bluetooth/hci_core.h +0 -0

 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);

net/bluetooth/Kconfig +0 -0

index 95d1a66..cba56c7 100644
@@ -69,6 +69,15 @@ config BT_6LOWPAN
 	help
 	  IPv6 compression over Bluetooth Low Energy.

Quoted 11 lines

net/bluetooth/Kconfig config BT_6LOWPAN · line 79
Quoted 8 lines
net/bluetooth/Kconfig +8 -0
+config BT_LEDS
+	bool "Enable LED triggers"
+	depends on BT
+	depends on LEDS_CLASS
+	select LEDS_TRIGGERS
+	help
+	 This option selects a few LED triggers for different
+	 Bluetooth events.
Not sure if the diff is messing this up, but just in case, two spaces extra indentation for the help text body.

Thanks for catching this.

Quoted 10 lines

net/bluetooth/Kconfig +1 -0

config BT_SELFTEST

	bool "Bluetooth self testing support"
	depends on BT && DEBUG_KERNEL

net/bluetooth/Makefile +0 -0

index 2b15ae8..98d8988 100644
@@ -17,6 +17,7 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \

Quoted 6 lines

Quoted 3 lines
bluetooth-$(CONFIG_BT_BREDR) += sco.o
bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
+bluetooth-$(CONFIG_BT_LEDS) += led.o
I would use plural leds.c here.

Quoted 82 lines

Quoted 71 lines

bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o

net/bluetooth/hci_core.c +8 -0

index 47bcef7..63a4229 100644
@@ -40,6 +40,7 @@
 #include "hci_request.h"
 #include "hci_debugfs.h"
 #include "smp.h"
+#include "led.h"
 
 static void hci_rx_work(struct work_struct *work);
 static void hci_cmd_work(struct work_struct *work);

@@ -1395,6 +1396,7 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 		hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
 		set_bit(HCI_UP, &hdev->flags);
 		hci_sock_dev_event(hdev, HCI_DEV_UP);
+		hci_led_radio(hdev, true);
 		if (!hci_dev_test_flag(hdev, HCI_SETUP) &&
 		    !hci_dev_test_flag(hdev, HCI_CONFIG) &&
 		    !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&

@@ -1532,6 +1534,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
 		return 0;
 	}
 
+	hci_led_radio(hdev, false);
+
 	/* Flush RX and TX works */
 	flush_work(&hdev->tx_work);
 	flush_work(&hdev->rx_work);

@@ -3067,6 +3071,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	if (error < 0)
 		goto err_wqueue;
 
+	hci_led_init(hdev);
+
 	hdev->rfkill = rfkill_alloc(hdev->name, &hdev->dev,
 				    RFKILL_TYPE_BLUETOOTH, &hci_rfkill_ops,
 				    hdev);

@@ -3128,6 +3134,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 
 	id = hdev->id;
 
+	hci_led_exit(hdev);
+
 	write_lock(&hci_dev_list_lock);
 	list_del(&hdev->list);
 	write_unlock(&hci_dev_list_lock);

net/bluetooth/hci_event.c +2 -0

index 7554da5..e5545c3 100644
@@ -35,6 +35,7 @@
 #include "a2mp.h"
 #include "amp.h"
 #include "smp.h"
+#include "led.h"
 
 #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
 		 "\x00\x00\x00\x00\x00\x00\x00\x00"

@@ -5239,6 +5240,7 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
 	case HCI_EV_CONN_COMPLETE:
 		hci_conn_complete_evt(hdev, skb);
+		hci_led_assoc(hdev, true);
 		break;
 
 	case HCI_EV_CONN_REQUEST:

net/bluetooth/hci_event.c void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb) · line 5253

Quoted 6 lines

net/bluetooth/hci_event.c +1 -0

@@ -5246,6 +5248,7 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		break;
 
 	case HCI_EV_DISCONN_COMPLETE:
+		hci_led_assoc(hdev, false);
 		hci_disconn_complete_evt(hdev, skb);
 		break;

You need to hook this up into hci_conn handling that actually represents logical connections.

Quoted 8 lines

I think this makes also pretty clear that assoc is a pretty bad word in the Bluetooth world. It has no meaning for BR/EDR or LE operation. You want a connection state LED.
Now the question becomes if you actually want a connection state LED or an actual activity LED. For BR/EDR you have the controller be discovering devices or being actually in a connection or both. For LE you have discovery as well (active scanning) and being in a connection state. However you have additionally background auto-connection (passive scanning) and also advertising.
Thinking this further through, you could actually be a lot more detailed in the LE world. Here you have scanning state, advertising state and connection state (maybe also initiating state). With the concept of multiple instances of the state machine, you could have a separate LED for each of these.
The radio on/off trigger is simple and I most likely would just start with that one, but for the "assoc" trigger this needs a bit more thinking since Bluetooth does work different than WiFi.

Thanks for the comprehensive explanation. I was expecting already that BT works quite different than WiFi. Then I'll follow the hint and start with the radio trigger only.

Quoted 65 lines

net/bluetooth/led.c new file +58 -0

new file mode 100644
index 0000000..420633c
@@ -0,0 +1,126 @@
+/*
+ * Copyright 2015, Heiner Kallweit <[email protected]>
+ * This code is based on the MAC80211 LED code.
+ * Original author: Johannes Berg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "led.h"
+
+#define cdev_to_hdev(cdev, name) container_of(cdev->trigger, \
+					      struct hci_dev, \
+					      name)
+
+void hci_led_assoc(struct hci_dev *hdev, bool associated)
+{
+	int cnt;
+
+	if (associated)
+		cnt = atomic_inc_return(&hdev->assoc_led_cnt);
+	else
+		cnt = atomic_dec_return(&hdev->assoc_led_cnt);
+
+	WARN_ON(cnt < 0);
+
+	if (!atomic_read(&hdev->assoc_led_active))
+		return;
+
+	led_trigger_event(&hdev->assoc_led,
+			  cnt ? LED_FULL : LED_OFF);
+}
+
+void hci_led_radio(struct hci_dev *hdev, bool enabled)
+{
+	int cnt;
+
+	if (enabled)
+		cnt = atomic_inc_return(&hdev->radio_led_cnt);
+	else
+		cnt = atomic_dec_return(&hdev->radio_led_cnt);
+
+	WARN_ON(cnt < 0);
+
+	if (!atomic_read(&hdev->radio_led_active))
+		return;
+
+	led_trigger_event(&hdev->radio_led,
+			  cnt ? LED_FULL : LED_OFF);
+}
+
+static void hci_assoc_led_activate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, assoc_led);

Quoted 9 lines

net/bluetooth/led.c line 64
Quoted 6 lines
net/bluetooth/led.c new file +6 -0
+	int cnt = atomic_read(&hdev->assoc_led_cnt);
+
+	atomic_inc(&hdev->assoc_led_active);
+
+	led_trigger_event(&hdev->assoc_led, cnt ? LED_FULL : LED_OFF);
+}
Why is this so complicated with the LED active handling? Isn't that something the LED class should actually take care of for us. What is the point in repeating that over and over again. Actually all of this should most likely have some sort of "simple cumulative LED" version that does that.

Right, meanwhile I had a closer look at the original mac80211 code and decided to more or less rewrite it. Will provide a v2 of the patch.

Quoted 47 lines

net/bluetooth/led.c new file +47 -0

+
+static void hci_assoc_led_deactivate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, assoc_led);
+
+	atomic_dec(&hdev->assoc_led_active);
+}
+
+static void hci_radio_led_activate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, radio_led);
+	int cnt = atomic_read(&hdev->radio_led_cnt);
+
+	atomic_inc(&hdev->radio_led_active);
+
+	led_trigger_event(&hdev->radio_led, cnt ? LED_FULL : LED_OFF);
+}
+
+static void hci_radio_led_deactivate(struct led_classdev *led_cdev)
+{
+	struct hci_dev *hdev = cdev_to_hdev(led_cdev, radio_led);
+
+	atomic_dec(&hdev->radio_led_active);
+}
+
+void hci_led_init(struct hci_dev *hdev)
+{
+	/* initialize assoc_led */
+	atomic_set(&hdev->assoc_led_cnt, 0);
+	atomic_set(&hdev->assoc_led_active, 0);
+	hdev->assoc_led.activate = hci_assoc_led_activate;
+	hdev->assoc_led.deactivate = hci_assoc_led_deactivate;
+	hdev->assoc_led.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+					      "%s-assoc", hdev->name);
+	if (hdev->assoc_led.name &&
+	    led_trigger_register(&hdev->assoc_led)) {
+		devm_kfree(&hdev->dev, (void *)hdev->assoc_led.name);
+		hdev->assoc_led.name = NULL;
+	}
+
+	/* initialize radio_led */
+	atomic_set(&hdev->radio_led_cnt, 0);
+	atomic_set(&hdev->radio_led_active, 0);
+	hdev->radio_led.activate = hci_radio_led_activate;
+	hdev->radio_led.deactivate = hci_radio_led_deactivate;
+	hdev->radio_led.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+					      "%s-radio", hdev->name);

Quoted 9 lines

net/bluetooth/led.c line 117
Quoted 6 lines
net/bluetooth/led.c new file +6 -0
+	if (hdev->radio_led.name &&
+	    led_trigger_register(&hdev->radio_led)) {
+		devm_kfree(&hdev->dev, (void *)hdev->radio_led.name);
+		hdev->radio_led.name = NULL;
+	}
+}
Now also the real question is if you just want a per controller LEDs or also some global LED that combines that status of all controllers attached to the system.

For the beginning I'd go with a per-controller LED.

Quoted 50 lines

Quoted 37 lines

net/bluetooth/led.c new file +9 -0

+
+void hci_led_exit(struct hci_dev *hdev)
+{
+	if (hdev->assoc_led.name)
+		led_trigger_unregister(&hdev->assoc_led);
+	if (hdev->radio_led.name)
+		led_trigger_unregister(&hdev->radio_led);
+}
+

net/bluetooth/led.h new file +22 -0

new file mode 100644
index 0000000..a607292
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2015, Heiner Kallweit <[email protected]>
+ * This code is based on the MAC80211 LED code.
+ * Original author: Johannes Berg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifdef CONFIG_BT_LEDS
+void hci_led_assoc(struct hci_dev *hdev, bool associated);
+void hci_led_radio(struct hci_dev *hdev, bool enabled);
+void hci_led_init(struct hci_dev *hdev);
+void hci_led_exit(struct hci_dev *hdev);
+#else
+static inline void hci_led_assoc(struct hci_dev *hdev, bool associated)
+{
+}
+
+static inline void hci_led_radio(struct hci_dev *hdev, bool enabled)
+{

net/bluetooth/led.h line 32

Quoted 10 lines

net/bluetooth/led.h new file +10 -0

+}
+
+static inline void hci_led_init(struct hci_dev *hdev)
+{
+}
+
+static inline void hci_led_exit(struct hci_dev *hdev)
+{
+}
+#endif

Regards

Quoted 2 lines

Marcel

Thanks for the fast and comprehensive review,
Heiner