[PATCH] brcmfmac: added LED triggers for transmit/receive

[PATCH] brcmfmac: added LED triggers for transmit/receive

[Russell Joyce]

Add three basic LED triggers to brcmfmac, based on those in mac80211: one
for transmit, one for receive, and one for combined transmit/receive.

Signed-off-by: Russell Joyce <russell.joyce@york.ac.uk>
---
 drivers/net/wireless/broadcom/brcm80211/Kconfig    |  12 +++
 .../wireless/broadcom/brcm80211/brcmfmac/Makefile  |   2 +
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         |   4 +
 .../broadcom/brcm80211/brcmfmac/cfg80211.h         |  16 +++
 .../wireless/broadcom/brcm80211/brcmfmac/core.c    |   5 +
 .../net/wireless/broadcom/brcm80211/brcmfmac/led.c | 113 +++++++++++++++++++++
 .../net/wireless/broadcom/brcm80211/brcmfmac/led.h |  57 +++++++++++
 7 files changed, 209 insertions(+)
 create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.c
 create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.h

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig b/drivers/net/wireless/broadcom/brcm80211/Kconfig
index 9d99eb42d917..7bb593aa755a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/Kconfig
+++ b/drivers/net/wireless/broadcom/brcm80211/Kconfig
@@ -68,6 +68,18 @@ config BRCMFMAC_PCIE
 	  IEEE802.11ac embedded FullMAC WLAN driver. Say Y if you want to
 	  use the driver for an PCIE wireless card.
 
+config BRCMFMAC_LEDS
+	bool "LED trigger support for FullMAC driver"
+	depends on BRCMFMAC
+	depends on LEDS_CLASS
+	select LEDS_TRIGGERS
+	default y
+	---help---
+	  This option enables LED trigger support for Broadcom FullMAC WLAN
+	  driver. Say Y to create LED triggers for receive events (phyXrx),
+	  transmit events (phyXtx), and both events combined (phyXrxtx), on
+	  each adapter (where 'phyX' is the phy name).
+
 config BRCM_TRACING
 	bool "Broadcom device tracing"
 	depends on BRCMSMAC || BRCMFMAC
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
index 1f5a9b948abf..5c33c582a75a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
@@ -48,6 +48,8 @@ brcmfmac-$(CONFIG_BRCMFMAC_USB) += \
 		usb.o
 brcmfmac-$(CONFIG_BRCMFMAC_PCIE) += \
 		pcie.o
+brcmfmac-$(CONFIG_BRCMFMAC_LEDS) += \
+		led.o
 brcmfmac-$(CONFIG_BRCMDBG) += \
 		debug.o
 brcmfmac-$(CONFIG_BRCM_TRACING) += \
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index dcde596c9eb9..43c028513aa2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -40,6 +40,7 @@
 #include "vendor.h"
 #include "bus.h"
 #include "common.h"
+#include "led.h"
 
 #define BRCMF_SCAN_IE_LEN_MAX		2048
 
@@ -7123,6 +7124,8 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 #endif
 	}
 
+	brcmfmac_led_init(cfg);
+
 	return cfg;
 
 detach:
@@ -7147,6 +7150,7 @@ void brcmf_cfg80211_detach(struct brcmf_cfg80211_info *cfg)
 	if (!cfg)
 		return;
 
+	brcmfmac_led_exit(cfg);
 	brcmf_pno_detach(cfg);
 	brcmf_btcoex_detach(cfg);
 	wiphy_unregister(cfg->wiphy);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
index 7b2835e5e434..304c13b409a1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
@@ -17,6 +17,8 @@
 #ifndef BRCMFMAC_CFG80211_H
 #define BRCMFMAC_CFG80211_H
 
+#include <linux/leds.h>
+
 /* for brcmu_d11inf */
 #include <brcmu_d11.h>
 
@@ -301,6 +303,12 @@ struct brcmf_cfg80211_wowl {
  * @vif_event: vif event signalling.
  * @wowl: wowl related information.
  * @pno: information of pno module.
+ * @rx_led: receive LED trigger information.
+ * @tx_led: transmit LED trigger information.
+ * @rxtx_led: receive/transmit LED trigger information.
+ * @rx_led_active: receive LED trigger active.
+ * @tx_led_active: transmit LED trigger active.
+ * @rxtx_led_active: receive/transmit LED trigger active.
  */
 struct brcmf_cfg80211_info {
 	struct wiphy *wiphy;
@@ -335,6 +343,14 @@ struct brcmf_cfg80211_info {
 	struct brcmf_assoclist_le assoclist;
 	struct brcmf_cfg80211_wowl wowl;
 	struct brcmf_pno_info *pno;
+#ifdef CONFIG_BRCMFMAC_LEDS
+	struct led_trigger rx_led;
+	struct led_trigger tx_led;
+	struct led_trigger rxtx_led;
+	atomic_t rx_led_active;
+	atomic_t tx_led_active;
+	atomic_t rxtx_led_active;
+#endif
 };
 
 /**
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 2153e8062b4c..8cc0bf111e27 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -37,6 +37,7 @@
 #include "proto.h"
 #include "pcie.h"
 #include "common.h"
+#include "led.h"
 
 #define MAX_WAIT_FOR_8021X_TX			msecs_to_jiffies(950)
 
@@ -336,6 +337,8 @@ void brcmf_rx_frame(struct device *dev, struct sk_buff *skb, bool handle_event)
 
 	brcmf_dbg(DATA, "Enter: %s: rxp=%p\n", dev_name(dev), skb);
 
+	brcmfmac_led_rx(drvr->config);
+
 	if (brcmf_rx_hdrpull(drvr, skb, &ifp))
 		return;
 
@@ -373,6 +376,8 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
 	eh = (struct ethhdr *)(txp->data);
 	type = ntohs(eh->h_proto);
 
+	brcmfmac_led_tx(ifp->drvr->config);
+
 	if (type == ETH_P_PAE) {
 		atomic_dec(&ifp->pend_8021x_cnt);
 		if (waitqueue_active(&ifp->pend_8021x_wait))
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.c
new file mode 100644
index 000000000000..d85ef430333d
--- /dev/null
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.c
@@ -0,0 +1,113 @@
+/*
+ * Copyright 2006, Johannes Berg <johannes@sipsolutions.net>
+ * Copyright 2017, Russell Joyce <russell.joyce@york.ac.uk>
+ *
+ * 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 <linux/slab.h>
+#include "led.h"
+
+static void brcmfmac_rx_led_activate(struct led_classdev *led_cdev)
+{
+	struct brcmf_cfg80211_info *info = container_of(led_cdev->trigger,
+						struct brcmf_cfg80211_info,
+						rx_led);
+
+	atomic_inc(&info->rx_led_active);
+}
+
+static void brcmfmac_rx_led_deactivate(struct led_classdev *led_cdev)
+{
+	struct brcmf_cfg80211_info *info = container_of(led_cdev->trigger,
+						struct brcmf_cfg80211_info,
+						rx_led);
+
+	atomic_dec(&info->rx_led_active);
+}
+
+static void brcmfmac_tx_led_activate(struct led_classdev *led_cdev)
+{
+	struct brcmf_cfg80211_info *info = container_of(led_cdev->trigger,
+						struct brcmf_cfg80211_info,
+						tx_led);
+
+	atomic_inc(&info->tx_led_active);
+}
+
+static void brcmfmac_tx_led_deactivate(struct led_classdev *led_cdev)
+{
+	struct brcmf_cfg80211_info *info = container_of(led_cdev->trigger,
+						struct brcmf_cfg80211_info,
+						tx_led);
+
+	atomic_dec(&info->tx_led_active);
+}
+
+static void brcmfmac_rxtx_led_activate(struct led_classdev *led_cdev)
+{
+	struct brcmf_cfg80211_info *info = container_of(led_cdev->trigger,
+						struct brcmf_cfg80211_info,
+						rxtx_led);
+
+	atomic_inc(&info->rxtx_led_active);
+}
+
+static void brcmfmac_rxtx_led_deactivate(struct led_classdev *led_cdev)
+{
+	struct brcmf_cfg80211_info *info = container_of(led_cdev->trigger,
+						struct brcmf_cfg80211_info,
+						rxtx_led);
+
+	atomic_dec(&info->rxtx_led_active);
+}
+
+void brcmfmac_led_init(struct brcmf_cfg80211_info *info)
+{
+	info->rx_led.name = kasprintf(GFP_KERNEL, "%srx",
+				      wiphy_name(info->wiphy));
+	info->tx_led.name = kasprintf(GFP_KERNEL, "%stx",
+				      wiphy_name(info->wiphy));
+	info->rxtx_led.name = kasprintf(GFP_KERNEL, "%srxtx",
+					wiphy_name(info->wiphy));
+
+	atomic_set(&info->rx_led_active, 0);
+	info->rx_led.activate = brcmfmac_rx_led_activate;
+	info->rx_led.deactivate = brcmfmac_rx_led_deactivate;
+	if (info->rx_led.name && led_trigger_register(&info->rx_led)) {
+		kfree(info->rx_led.name);
+		info->rx_led.name = NULL;
+	}
+
+	atomic_set(&info->tx_led_active, 0);
+	info->tx_led.activate = brcmfmac_tx_led_activate;
+	info->tx_led.deactivate = brcmfmac_tx_led_deactivate;
+	if (info->tx_led.name && led_trigger_register(&info->tx_led)) {
+		kfree(info->tx_led.name);
+		info->tx_led.name = NULL;
+	}
+
+	atomic_set(&info->rxtx_led_active, 0);
+	info->rxtx_led.activate = brcmfmac_rxtx_led_activate;
+	info->rxtx_led.deactivate = brcmfmac_rxtx_led_deactivate;
+	if (info->rxtx_led.name && led_trigger_register(&info->rxtx_led)) {
+		kfree(info->rxtx_led.name);
+		info->rxtx_led.name = NULL;
+	}
+}
+
+void brcmfmac_led_exit(struct brcmf_cfg80211_info *info)
+{
+	if (info->rx_led.name)
+		led_trigger_unregister(&info->rx_led);
+	if (info->tx_led.name)
+		led_trigger_unregister(&info->tx_led);
+	if (info->rxtx_led.name)
+		led_trigger_unregister(&info->rxtx_led);
+
+	kfree(info->rx_led.name);
+	kfree(info->tx_led.name);
+	kfree(info->rxtx_led.name);
+}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.h
new file mode 100644
index 000000000000..c9eee5f8f89b
--- /dev/null
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/led.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright 2006, Johannes Berg <johannes@sipsolutions.net>
+ * Copyright 2017, Russell Joyce <russell.joyce@york.ac.uk>
+ *
+ * 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 <linux/atomic.h>
+#include <linux/leds.h>
+#include "cfg80211.h"
+
+#define BRCMFMAC_BLINK_DELAY 50 /* ms */
+
+static inline void brcmfmac_led_rx(struct brcmf_cfg80211_info *info)
+{
+#ifdef CONFIG_BRCMFMAC_LEDS
+	unsigned long led_delay = BRCMFMAC_BLINK_DELAY;
+
+	if (atomic_read(&info->rx_led_active))
+		led_trigger_blink_oneshot(&info->rx_led, &led_delay,
+					  &led_delay, 0);
+
+	if (atomic_read(&info->rxtx_led_active))
+		led_trigger_blink_oneshot(&info->rxtx_led, &led_delay,
+					  &led_delay, 0);
+#endif
+}
+
+static inline void brcmfmac_led_tx(struct brcmf_cfg80211_info *info)
+{
+#ifdef CONFIG_BRCMFMAC_LEDS
+	unsigned long led_delay = BRCMFMAC_BLINK_DELAY;
+
+	if (atomic_read(&info->tx_led_active))
+		led_trigger_blink_oneshot(&info->tx_led, &led_delay,
+					  &led_delay, 0);
+
+	if (atomic_read(&info->rxtx_led_active))
+		led_trigger_blink_oneshot(&info->rxtx_led, &led_delay,
+					  &led_delay, 0);
+#endif
+}
+
+#ifdef CONFIG_BRCMFMAC_LEDS
+void brcmfmac_led_init(struct brcmf_cfg80211_info *info);
+void brcmfmac_led_exit(struct brcmf_cfg80211_info *info);
+#else
+static inline void brcmfmac_led_init(struct brcmf_cfg80211_info *info)
+{
+}
+
+static inline void brcmfmac_led_exit(struct brcmf_cfg80211_info *info)
+{
+}
+#endif
-- 
2.11.0

Re: [PATCH] brcmfmac: added LED triggers for transmit/receive

[Rafał Miłecki]

On 7 July 2017 at 16:09, Russell Joyce <russell.joyce@york.ac.uk> wrote:
> Add three basic LED triggers to brcmfmac, based on those in mac80211: one
> for transmit, one for receive, and one for combined transmit/receive.
>
> Signed-off-by: Russell Joyce <russell.joyce@york.ac.uk>

1) I think most of it should be some cfg80211 shareable code.
2) This "rxtx" while surely present in other places sounds like a
workaround for LED subsystem limitation. Maybe it's time to finally
rework LED triggers.

Re: [PATCH] brcmfmac: added LED triggers for transmit/receive

[Russell Joyce]

> 1) I think most of it should be some cfg80211 shareable code.

I=E2=80=99m not sure exactly what you mean by this, could you please =
clarify?

> 2) This "rxtx" while surely present in other places sounds like a
> workaround for LED subsystem limitation. Maybe it's time to finally
> rework LED triggers.

I agree that it=E2=80=99s not an ideal way to do things, but I =
couldn=E2=80=99t think of a
better alternative. I think that having a combined trigger is useful =
though, for
situations like using the single LED on a Raspberry Pi to show Wi-Fi =
activity.


> On 10 Jul 2017, at 10:48, Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com> =
wrote:
>=20
> On 7 July 2017 at 16:09, Russell Joyce <russell.joyce@york.ac.uk> =
wrote:
>> Add three basic LED triggers to brcmfmac, based on those in mac80211: =
one
>> for transmit, one for receive, and one for combined transmit/receive.
>>=20
>> Signed-off-by: Russell Joyce <russell.joyce@york.ac.uk>
>=20
> 1) I think most of it should be some cfg80211 shareable code.
> 2) This "rxtx" while surely present in other places sounds like a
> workaround for LED subsystem limitation. Maybe it's time to finally
> rework LED triggers.

Re: [PATCH] brcmfmac: added LED triggers for transmit/receive

[Arend van Spriel]

On 10-07-17 19:02, Russell Joyce wrote:
>> 1) I think most of it should be some cfg80211 shareable code.
> 
> I’m not sure exactly what you mean by this, could you please clarify?

What I think Rafał is saying is that it would be better to have this
code in cfg80211 so other drivers including mac80211 could use it.

>> 2) This "rxtx" while surely present in other places sounds like a
>> workaround for LED subsystem limitation. Maybe it's time to finally
>> rework LED triggers.
> 
> I agree that it’s not an ideal way to do things, but I couldn’t think of a
> better alternative. I think that having a combined trigger is useful though, for
> situations like using the single LED on a Raspberry Pi to show Wi-Fi activity.

Indeed. However, the LED subsystem could/should(?) take care of mapping
"rx" and "tx" triggers to the same LED.

I am happy to comment on your patch, but maybe you can first take a look
at the suggestion Rafał made above.

Regards,
Arend

Re: [PATCH] brcmfmac: added LED triggers for transmit/receive

[Russell Joyce]

Thanks for your comments.

> What I think Rafa=C5=82 is saying is that it would be better to have =
this
> code in cfg80211 so other drivers including mac80211 could use it.


While I agree that moving all wireless LED triggers to cfg80211 would be =
an
ideal situation, it seems a bit out of scope for what I was trying to =
achieve.
This would probably also require removing the mac80211 LED triggers (and =
any
other similar triggers that might be created by specific wireless =
drivers not
using mac80211), in order to consolidate them in one place.

Besides this, I'm not sure where exactly in cfg80211 this functionality =
would
go (I assume it was originally put in mac80211 instead for a reason?),
although I'm certainly no expert in this area of the kernel.

> Indeed. However, the LED subsystem could/should(?) take care of =
mapping
> "rx" and "tx" triggers to the same LED.

In terms of the LED triggers, the only alternative I can see is to =
create a
single complex trigger that exposes "rx" and "tx" parameters that can be
individually enabled or disabled. This would reduce the number of =
triggers from
three to one, but also makes things slightly more awkward for the user, =
and
deviates from the convention set by mac80211.

Re: [PATCH] brcmfmac: added LED triggers for transmit/receive

[Rafał Miłecki]

On 11 July 2017 at 17:01, Russell Joyce <russell.joyce@york.ac.uk> wrote:
> Thanks for your comments.
>
>> What I think Rafa=C5=82 is saying is that it would be better to have thi=
s
>> code in cfg80211 so other drivers including mac80211 could use it.
>
>
> While I agree that moving all wireless LED triggers to cfg80211 would be =
an
> ideal situation, it seems a bit out of scope for what I was trying to ach=
ieve.
> This would probably also require removing the mac80211 LED triggers (and =
any
> other similar triggers that might be created by specific wireless drivers=
 not
> using mac80211), in order to consolidate them in one place.
>
> Besides this, I'm not sure where exactly in cfg80211 this functionality w=
ould
> go (I assume it was originally put in mac80211 instead for a reason?),
> although I'm certainly no expert in this area of the kernel.

I don't expect you to rewrite all mac80211 drivers at this point. Just
focus on the generic cfg80211 helper and use it in brcmfmac. Other
cfg80211 drivers and mac80211 may follow in the future.

I'm not sure what's the best place in cfg80211 for this. Try
something, or try to get some comments from cfg80211 guys.


>> Indeed. However, the LED subsystem could/should(?) take care of mapping
>> "rx" and "tx" triggers to the same LED.
>
> In terms of the LED triggers, the only alternative I can see is to create=
 a
> single complex trigger that exposes "rx" and "tx" parameters that can be
> individually enabled or disabled. This would reduce the number of trigger=
s from
> three to one, but also makes things slightly more awkward for the user, a=
nd
> deviates from the convention set by mac80211.

Ideally we should have "rx" and "tx". LED subsystem should allow
assigning *both* (at the same time) to the LED.

I'll try to discuss with with LED guys this week.

Sorry, I was busy last week.

--=20
Rafa=C5=82