[PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
packets available in tx or rx, the LEDs will blink.

For each hci registration, two triggers are added into LED subsystem:
[hdev->name]-tx and [hdev-name]-rx.
Refer to Documentation/leds/leds-class.txt for usage.

Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
WL1835 WiFi/BT combo chip.

Signed-off-by: Guodong Xu <redacted>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         |  6 ++++++
 net/bluetooth/leds.c             | 17 +++++++++++++++++
 net/bluetooth/leds.h             |  2 ++
 4 files changed, 26 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index dc71473..37b8dd9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h

@@ -398,6 +398,7 @@ struct hci_dev {
 	bdaddr_t		rpa;
 
 	struct led_trigger	*power_led;
+	struct led_trigger	*tx_led, *rx_led;
 
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 45a9fc6..956dce1 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c

@@ -3833,6 +3833,7 @@ static void hci_sched_acl(struct hci_dev *hdev)
 	if (!hci_conn_num(hdev, AMP_LINK) && hdev->dev_type == HCI_AMP)
 		return;
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	switch (hdev->flow_ctl_mode) {
 	case HCI_FLOW_CTL_MODE_PACKET_BASED:
 		hci_sched_acl_pkt(hdev);

@@ -3856,6 +3857,7 @@ static void hci_sched_sco(struct hci_dev *hdev)
 	if (!hci_conn_num(hdev, SCO_LINK))
 		return;
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);

@@ -3879,6 +3881,7 @@ static void hci_sched_esco(struct hci_dev *hdev)
 	if (!hci_conn_num(hdev, ESCO_LINK))
 		return;
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
 						     &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {

@@ -3911,6 +3914,7 @@ static void hci_sched_le(struct hci_dev *hdev)
 			hci_link_tx_to(hdev, LE_LINK);
 	}
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
 	tmp = cnt;
 	while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, &quote))) {

@@ -3990,6 +3994,7 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (conn) {
 		hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF);
+		hci_leds_blink_oneshot(hdev->rx_led);
 
 		/* Send to upper protocol */
 		l2cap_recv_acldata(conn, skb, flags);

@@ -4022,6 +4027,7 @@ static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_dev_unlock(hdev);
 
 	if (conn) {
+		hci_leds_blink_oneshot(hdev->rx_led);
 		/* Send to upper protocol */
 		sco_recv_scodata(conn, skb);
 		return;

diff --git a/net/bluetooth/leds.c b/net/bluetooth/leds.c
index 8319c84..ae10c5d 100644
--- a/net/bluetooth/leds.c
+++ b/net/bluetooth/leds.c

@@ -19,6 +19,8 @@ struct hci_basic_led_trigger {
 #define to_hci_basic_led_trigger(arg) container_of(arg, \
 			struct hci_basic_led_trigger, led_trigger)
 
+#define BLUETOOTH_BLINK_DELAY	50 /* ms */
+
 void hci_leds_update_powered(struct hci_dev *hdev, bool enabled)
 {
 	if (hdev->power_led)

@@ -37,6 +39,17 @@ static void power_activate(struct led_classdev *led_cdev)
 	led_trigger_event(led_cdev->trigger, powered ? LED_FULL : LED_OFF);
 }
 
+void hci_leds_blink_oneshot(struct led_trigger *trig)
+{
+	unsigned long led_delay = BLUETOOTH_BLINK_DELAY;
+
+	if (!trig)
+		return;
+
+	BT_DBG("led_trig %p", trig);
+	led_trigger_blink_oneshot(trig, &led_delay, &led_delay, 0);
+}
+
 static struct led_trigger *led_allocate_basic(struct hci_dev *hdev,
 			void (*activate)(struct led_classdev *led_cdev),
 			const char *name)

@@ -71,4 +84,8 @@ void hci_leds_init(struct hci_dev *hdev)
 {
 	/* initialize power_led */
 	hdev->power_led = led_allocate_basic(hdev, power_activate, "power");
+	/* initialize tx_led */
+	hdev->tx_led = led_allocate_basic(hdev, NULL, "tx");
+	/* initialize rx_led */
+	hdev->rx_led = led_allocate_basic(hdev, NULL, "rx");
 }

diff --git a/net/bluetooth/leds.h b/net/bluetooth/leds.h
index a9c4d6e..9b1cccd 100644
--- a/net/bluetooth/leds.h
+++ b/net/bluetooth/leds.h

@@ -9,8 +9,10 @@
 #if IS_ENABLED(CONFIG_BT_LEDS)
 void hci_leds_update_powered(struct hci_dev *hdev, bool enabled);
 void hci_leds_init(struct hci_dev *hdev);
+void hci_leds_blink_oneshot(struct led_trigger *trig);
 #else
 static inline void hci_leds_update_powered(struct hci_dev *hdev,
 					   bool enabled) {}
 static inline void hci_leds_init(struct hci_dev *hdev) {}
+static inline void hci_leds_blink_oneshot(struct led_trigger *trig) {}
 #endif

-- 
1.9.1

Ping. :)  Would you give me some review opinions on this?

In this revision, I chose to limit LED blinking to tx/rx traffic
packets only. For other types of over-the-air packets, like scanning,
it's not covered.

Thank you.

-Guodong


On 31 July 2016 at 12:24, Guodong Xu [off-list ref] wrote:

Hi Guodong,

Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
packets available in tx or rx, the LEDs will blink.

For each hci registration, two triggers are added into LED subsystem:
[hdev->name]-tx and [hdev-name]-rx.
Refer to Documentation/leds/leds-class.txt for usage.

Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
WL1835 WiFi/BT combo chip.

so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers. If we then maybe add another trigger, then this number just goes up and up.

As far as I can tell you can only assign a single trigger to a LED. So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.

Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.

So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?

Regards

Marcel

Hi, Marcel

On 16 August 2016 at 14:03, Marcel Holtmann [off-list ref] wrote:

Hi Guodong,

quoted

Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
packets available in tx or rx, the LEDs will blink.

For each hci registration, two triggers are added into LED subsystem:
[hdev->name]-tx and [hdev-name]-rx.
Refer to Documentation/leds/leds-class.txt for usage.

Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
WL1835 WiFi/BT combo chip.

so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.

True, 6 triggers. But, taking example for other subsytems, eg. cpu
cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
cpu6 cpu7". It doesn't have to mean you need all of them connected to
some LED(s). Actually, in most of the case, I only need heartbeat.

If we then maybe add another trigger, then this number just goes up and up.

As far as I can tell you can only assign a single trigger to a LED.

That's true. And people got a choice of which feature he wants to visualize.

So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.

Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.

So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?

When I starting this work, I referred to WiFi system. See
CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.

Besides, there are also RFKILL which stands for WiFi/BT power status.
RFKILL adds triggers for each module too. Eg. in the below example, I
have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
hci0-power.

Ref: here are all LED triggers I found in my 96boards/HiKey:

# cat trigger
none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
hci0-power hci0-tx [hci0-rx] rfkill1

-Guodong

Regards

Marcel

Hi Guodong,

quoted

quoted

Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
packets available in tx or rx, the LEDs will blink.

For each hci registration, two triggers are added into LED subsystem:
[hdev->name]-tx and [hdev-name]-rx.
Refer to Documentation/leds/leds-class.txt for usage.

Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
WL1835 WiFi/BT combo chip.

so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.

True, 6 triggers. But, taking example for other subsytems, eg. cpu
cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
cpu6 cpu7". It doesn't have to mean you need all of them connected to
some LED(s). Actually, in most of the case, I only need heartbeat.

quoted

If we then maybe add another trigger, then this number just goes up and up.

As far as I can tell you can only assign a single trigger to a LED.

That's true. And people got a choice of which feature he wants to visualize.

and as a result we keep adding senseless triggers to the kernel and bloating it up for no reason. Especially since it feels like 99% of the LED triggers are not used at all. This makes no sense to me.

quoted

So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.

Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.

So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?

When I starting this work, I referred to WiFi system. See
CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.

And I actually wonder who ever used these triggers. You need 4 LEDs to visualize the WiFi status. Which systems has 4 LEDs to spare to visualize this.

Besides, there are also RFKILL which stands for WiFi/BT power status.
RFKILL adds triggers for each module too. Eg. in the below example, I
have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
hci0-power.

Ref: here are all LED triggers I found in my 96boards/HiKey:

# cat trigger
none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
hci0-power hci0-tx [hci0-rx] rfkill1

And how many LEDs do you have in the your system? I think you are making my point here.

So I think what we need to do is to not add to this madness and instead create one "bluetooth" LED trigger that combines power and TX/RX for all controllers. And then allow for individual "bluetooth-hci0" LED triggers so that you can bind a single Bluetooth controller to a single LED.

For me, if I can not combine hci0-power, hci0-tx and hci0-rx into a single LED, it becomes utterly useless on pretty much every system that is out there.

Regards

Marcel

On 16 August 2016 at 20:33, Marcel Holtmann [off-list ref] wrote:

Hi Guodong,

quoted

quoted

quoted

Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
packets available in tx or rx, the LEDs will blink.

For each hci registration, two triggers are added into LED subsystem:
[hdev->name]-tx and [hdev-name]-rx.
Refer to Documentation/leds/leds-class.txt for usage.

Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
WL1835 WiFi/BT combo chip.

so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.

True, 6 triggers. But, taking example for other subsytems, eg. cpu
cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
cpu6 cpu7". It doesn't have to mean you need all of them connected to
some LED(s). Actually, in most of the case, I only need heartbeat.

quoted

If we then maybe add another trigger, then this number just goes up and up.

As far as I can tell you can only assign a single trigger to a LED.

That's true. And people got a choice of which feature he wants to visualize.

and as a result we keep adding senseless triggers to the kernel and bloating it up for no reason. Especially since it feels like 99% of the LED triggers are not used at all. This makes no sense to me.

quoted

quoted

So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.

Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.

So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?

When I starting this work, I referred to WiFi system. See
CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.

And I actually wonder who ever used these triggers. You need 4 LEDs to visualize the WiFi status. Which systems has 4 LEDs to spare to visualize this.

quoted

Besides, there are also RFKILL which stands for WiFi/BT power status.
RFKILL adds triggers for each module too. Eg. in the below example, I
have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
hci0-power.

Ref: here are all LED triggers I found in my 96boards/HiKey:

# cat trigger
none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
hci0-power hci0-tx [hci0-rx] rfkill1

And how many LEDs do you have in the your system? I think you are making my point here.

So I think what we need to do is to not add to this madness and instead create one "bluetooth" LED trigger that combines power and TX/RX for all controllers. And then allow for individual "bluetooth-hci0" LED triggers so that you can bind a single Bluetooth controller to a single LED.

For me, if I can not combine hci0-power, hci0-tx and hci0-rx into a single LED,

By combining them into a single LED, do you mean such a use case?
 - when hci0 is powered on, this LED starts on.
 - then, when there is tx/rx traffic, this LED should blink (reversely
of course).
 - when hci0 is powered off, this LED turns off.

-Guodong

it becomes utterly useless on pretty much every system that is out there.

Regards

Marcel

Hi Guodong,

quoted

quoted

quoted

quoted

Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
packets available in tx or rx, the LEDs will blink.

For each hci registration, two triggers are added into LED subsystem:
[hdev->name]-tx and [hdev-name]-rx.
Refer to Documentation/leds/leds-class.txt for usage.

Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
WL1835 WiFi/BT combo chip.

so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.

True, 6 triggers. But, taking example for other subsytems, eg. cpu
cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
cpu6 cpu7". It doesn't have to mean you need all of them connected to
some LED(s). Actually, in most of the case, I only need heartbeat.

quoted

If we then maybe add another trigger, then this number just goes up and up.

As far as I can tell you can only assign a single trigger to a LED.

That's true. And people got a choice of which feature he wants to visualize.

and as a result we keep adding senseless triggers to the kernel and bloating it up for no reason. Especially since it feels like 99% of the LED triggers are not used at all. This makes no sense to me.

quoted

quoted

So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.

Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.

So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?

When I starting this work, I referred to WiFi system. See
CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.

And I actually wonder who ever used these triggers. You need 4 LEDs to visualize the WiFi status. Which systems has 4 LEDs to spare to visualize this.

quoted

Besides, there are also RFKILL which stands for WiFi/BT power status.
RFKILL adds triggers for each module too. Eg. in the below example, I
have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
hci0-power.

Ref: here are all LED triggers I found in my 96boards/HiKey:

# cat trigger
none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
hci0-power hci0-tx [hci0-rx] rfkill1

And how many LEDs do you have in the your system? I think you are making my point here.

So I think what we need to do is to not add to this madness and instead create one "bluetooth" LED trigger that combines power and TX/RX for all controllers. And then allow for individual "bluetooth-hci0" LED triggers so that you can bind a single Bluetooth controller to a single LED.

For me, if I can not combine hci0-power, hci0-tx and hci0-rx into a single LED,

By combining them into a single LED, do you mean such a use case?
- when hci0 is powered on, this LED starts on.
- then, when there is tx/rx traffic, this LED should blink (reversely
of course).
- when hci0 is powered off, this LED turns off.

yes, that is what I am thinking of.

Regards

Marcel