This is another attempt on adding this feature on LEDs.
Most of the times Switch/PHY have connected multiple LEDs that are
controlled by HW based on some rules/event. Currently we lack any
support for a generic way to control the HW part and normally we
either never implement the feature or only add control for brightness
or hw blink.
This is based on Marek idea of providing some API to cled but use a
different implementation that in theory should be more generilized.
The current idea is:
- LED driver implement 3 API (hw_control_status/start/stop).
They are used to put the LED in hardware mode and to configure the
various trigger.
- We have hardware triggers that are used to expose to userspace the
supported hardware mode and set the hardware mode on trigger
activation.
- We can also have triggers that both support hardware and software mode.
- The LED driver will declare each supported hardware blink mode and
communicate with the trigger all the supported blink modes that will
be available by sysfs.
- A trigger will use blink_set to configure the blink mode to active
in hardware mode.
- On hardware trigger activation, only the hardware mode is enabled but
the blink modes are not configured. The LED driver should reset any
link mode active by default.
Each LED driver will have to declare explicit support for the offload
trigger (or return not supported error code) as we the trigger_data that
the LED driver will elaborate and understand what is referring to (based
on the current active trigger).
I posted a user for this new implementation that will benefit from this
and will add a big feature to it. Currently qca8k can have up to 3 LEDs
connected to each PHY port and we have some device that have only one of
them connected and the default configuration won't work for that.
The netdev trigger is expanded and it does now support hardware only
triggers.
The idea is to use hardware mode when a device_name is not defined.
An additional sysfs entry is added to give some info about the available
trigger modes supported in the current configuration.
More polish is required but this is just to understand if I'm taking
the correct path with this implementation hoping we find a correct
implementation and we start working on the ""small details""
v6:
- Back to RFC.
- Drop additional trigger
- Rework netdev trigger to support common modes used by switch and
hardware only triggers
- Refresh qca8k leds logic and driver
v5:
- Move out of RFC. (no comments from Andrew this is the right path?)
- Fix more spelling mistake (thx Randy)
- Fix error reported by kernel test bot
- Drop the additional HW_CONTROL flag. It does simplify CONFIG
handling and hw control should be available anyway to support
triggers as module.
v4:
- Rework implementation and drop hw_configure logic.
We now expand blink_set.
- Address even more spelling mistake. (thx a lot Randy)
- Drop blink option and use blink_set delay.
- Rework phy-activity trigger to actually make the groups dynamic.
v3:
- Rework start/stop as Andrew asked.
- Introduce more logic to permit a trigger to run in hardware mode.
- Add additional patch with netdev hardware support.
- Use test_bit API to check flag passed to hw_control_configure.
- Added a new cmd to hw_control_configure to reset any active blink_mode.
- Refactor all the patches to follow this new implementation.
v2:
- Fix spelling mistake (sorry)
- Drop patch 02 "permit to declare supported offload triggers".
Change the logic, now the LED driver declare support for them
using the configure_offload with the cmd TRIGGER_SUPPORTED.
- Rework code to follow this new implementation.
- Update Documentation to better describe how this offload
implementation work.
Ansuel Smith (11):
leds: add support for hardware driven LEDs
leds: add function to configure hardware controlled LED
leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
leds: trigger: netdev: rename and expose NETDEV trigger enum modes
leds: trigger: netdev: convert device attr to macro
leds: trigger: netdev: add hardware control support
leds: trigger: netdev: use mutex instead of spinlocks
leds: trigger: netdev: add available mode sysfs attr
leds: trigger: netdev: add additional hardware only triggers
net: dsa: qca8k: add LEDs support
dt-bindings: net: dsa: qca8k: add LEDs definition example
.../devicetree/bindings/net/dsa/qca8k.yaml | 20 +
Documentation/leds/leds-class.rst | 53 +++
drivers/leds/Kconfig | 11 +
drivers/leds/led-class.c | 27 ++
drivers/leds/led-triggers.c | 29 ++
drivers/leds/trigger/ledtrig-netdev.c | 385 ++++++++++++-----
drivers/net/dsa/Kconfig | 9 +
drivers/net/dsa/Makefile | 1 +
drivers/net/dsa/qca8k-leds.c | 408 ++++++++++++++++++
drivers/net/dsa/qca8k.c | 4 +
drivers/net/dsa/qca8k.h | 61 +++
include/linux/leds.h | 103 ++++-
12 files changed, 1012 insertions(+), 99 deletions(-)
create mode 100644 drivers/net/dsa/qca8k-leds.c
--
2.34.1
Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
that will be true or false based on the carrier link. No functional
change intended.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..66a81cc9b64d 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -50,10 +50,10 @@ struct led_netdev_data {
unsigned int last_activity;
unsigned long mode;
+ bool carrier_link_up;
#define NETDEV_LED_LINK 0
#define NETDEV_LED_TX 1
#define NETDEV_LED_RX 2
-#define NETDEV_LED_MODE_LINKUP 3
};
enum netdev_led_attr {
@@ -73,9 +73,9 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
if (!led_cdev->blink_brightness)
led_cdev->blink_brightness = led_cdev->max_brightness;
- if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
+ if (!trigger_data->carrier_link_up) {
led_set_brightness(led_cdev, LED_OFF);
- else {
+ } else {
if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
led_set_brightness(led_cdev,
led_cdev->blink_brightness);
@@ -131,10 +131,9 @@ static ssize_t device_name_store(struct device *dev,
trigger_data->net_dev =
dev_get_by_name(&init_net, trigger_data->device_name);
- clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ trigger_data->carrier_link_up = false;
if (trigger_data->net_dev != NULL)
- if (netif_carrier_ok(trigger_data->net_dev))
- set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
trigger_data->last_activity = 0;
@@ -315,7 +314,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
spin_lock_bh(&trigger_data->lock);
- clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ trigger_data->carrier_link_up = false;
switch (evt) {
case NETDEV_CHANGENAME:
case NETDEV_REGISTER:
@@ -330,8 +329,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
break;
case NETDEV_UP:
case NETDEV_CHANGE:
- if (netif_carrier_ok(dev))
- set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ trigger_data->carrier_link_up = netif_carrier_ok(dev);
break;
}
--
2.34.1
Add LEDs definition example for qca8k using the offload trigger as the
default trigger and add all the supported offload triggers by the
switch.
Signed-off-by: Ansuel Smith <[email protected]>
---
.../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index f3c88371d76c..9b46ef645a2d 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -65,6 +65,8 @@ properties:
internal mdio access is used.
With the legacy mapping the reg corresponding to the internal
mdio is the switch reg with an offset of -1.
+ Each phy have at least 3 LEDs connected and can be declared
+ using the standard LEDs structure.
patternProperties:
"^(ethernet-)?ports$":
@@ -287,6 +289,24 @@ examples:
internal_phy_port1: ethernet-phy@0 {
reg = <0>;
+
+ leds {
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "netdev";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "netdev";
+ };
+ };
};
internal_phy_port2: ethernet-phy@1 {
--
2.34.1
Convert link tx and rx device attr to a common macro to reduce common
code and in preparation for additional attr.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 57 ++++++++-------------------
1 file changed, 16 insertions(+), 41 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 6872da08676b..dd63cadb896e 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -189,47 +189,22 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
return size;
}
-static ssize_t link_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return netdev_led_attr_show(dev, buf, NETDEV_ATTR_LINK);
-}
-
-static ssize_t link_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
-{
- return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_LINK);
-}
-
-static DEVICE_ATTR_RW(link);
-
-static ssize_t tx_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return netdev_led_attr_show(dev, buf, NETDEV_ATTR_TX);
-}
-
-static ssize_t tx_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
-{
- return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_TX);
-}
-
-static DEVICE_ATTR_RW(tx);
-
-static ssize_t rx_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return netdev_led_attr_show(dev, buf, NETDEV_ATTR_RX);
-}
-
-static ssize_t rx_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
-{
- return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_RX);
-}
-
-static DEVICE_ATTR_RW(rx);
+#define DEFINE_NETDEV_TRIGGER(trigger_name, trigger) \
+ static ssize_t trigger_name##_show(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+ { \
+ return netdev_led_attr_show(dev, buf, trigger); \
+ } \
+ static ssize_t trigger_name##_store(struct device *dev, \
+ struct device_attribute *attr, const char *buf, size_t size) \
+ { \
+ return netdev_led_attr_store(dev, buf, size, trigger); \
+ } \
+ static DEVICE_ATTR_RW(trigger_name)
+
+DEFINE_NETDEV_TRIGGER(link, TRIGGER_NETDEV_LINK);
+DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
+DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
static ssize_t interval_show(struct device *dev,
struct device_attribute *attr, char *buf)
--
2.34.1
Add hardware control support for the Netdev trigger.
The trigger on config change will check if the requested trigger can set
to blink mode using LED hardware mode and if every blink mode is supported,
the trigger will enable hardware mode with the requested configuration.
If there is at least one trigger that is not supported and can't run in
hardware mode, then software mode will be used instead.
A validation is done on every value change and on fail the old value is
restored and -EINVAL is returned.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
1 file changed, 149 insertions(+), 6 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index dd63cadb896e..ed019cb5867c 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -37,6 +37,7 @@
*/
struct led_netdev_data {
+ enum led_blink_modes blink_mode;
spinlock_t lock;
struct delayed_work work;
@@ -53,11 +54,105 @@ struct led_netdev_data {
bool carrier_link_up;
};
+struct netdev_led_attr_detail {
+ char *name;
+ bool hardware_only;
+ enum led_trigger_netdev_modes bit;
+};
+
+static struct netdev_led_attr_detail attr_details[] = {
+ { .name = "link", .bit = TRIGGER_NETDEV_LINK},
+ { .name = "tx", .bit = TRIGGER_NETDEV_TX},
+ { .name = "rx", .bit = TRIGGER_NETDEV_RX},
+};
+
+static bool validate_baseline_state(struct led_netdev_data *trigger_data)
+{
+ struct led_classdev *led_cdev = trigger_data->led_cdev;
+ struct netdev_led_attr_detail *detail;
+ u32 hw_blink_mode_supported = 0;
+ bool force_sw = false;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+ detail = &attr_details[i];
+
+ /* Mode not active, skip */
+ if (!test_bit(detail->bit, &trigger_data->mode))
+ continue;
+
+ /* Hardware only mode enabled on software controlled led */
+ if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
+ detail->hardware_only)
+ return false;
+
+ /* Check if the mode supports hardware mode */
+ if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
+ /* With a net dev set, force software mode.
+ * With modes are handled by hardware, led will blink
+ * based on his own events and will ignore any event
+ * from the provided dev.
+ */
+ if (trigger_data->net_dev) {
+ force_sw = true;
+ continue;
+ }
+
+ /* With empty dev, check if the mode is supported */
+ if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
+ hw_blink_mode_supported |= BIT(detail->bit);
+ }
+ }
+
+ /* We can't run modes handled by both software and hardware.
+ * Check if we run hardware modes and check if all the modes
+ * can be handled by hardware.
+ */
+ if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
+ return false;
+
+ /* Modes are valid. Decide now the running mode to later
+ * set the baseline.
+ * Software mode is enforced with net_dev set. With an empty
+ * one hardware mode is selected by default (if supported).
+ */
+ if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
+ trigger_data->blink_mode = SOFTWARE_CONTROLLED;
+ else
+ trigger_data->blink_mode = HARDWARE_CONTROLLED;
+
+ return true;
+}
+
static void set_baseline_state(struct led_netdev_data *trigger_data)
{
+ int i;
int current_brightness;
+ struct netdev_led_attr_detail *detail;
struct led_classdev *led_cdev = trigger_data->led_cdev;
+ /* Modes already validated. Directly apply hw trigger modes */
+ if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
+ /* We are refreshing the blink modes. Reset them */
+ led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
+ BLINK_MODE_ZERO);
+
+ for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+ detail = &attr_details[i];
+
+ if (!test_bit(detail->bit, &trigger_data->mode))
+ continue;
+
+ led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
+ BLINK_MODE_ENABLE);
+ }
+
+ led_cdev->hw_control_start(led_cdev);
+
+ return;
+ }
+
+ /* Handle trigger modes by software */
current_brightness = led_cdev->brightness;
if (current_brightness)
led_cdev->blink_brightness = current_brightness;
@@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
size_t size)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ struct net_device *old_net = trigger_data->net_dev;
+ char old_device_name[IFNAMSIZ];
if (size >= IFNAMSIZ)
return -EINVAL;
+ /* Backup old device name */
+ memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
+
cancel_delayed_work_sync(&trigger_data->work);
spin_lock_bh(&trigger_data->lock);
@@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
trigger_data->net_dev =
dev_get_by_name(&init_net, trigger_data->device_name);
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old net_dev and device_name */
+ if (trigger_data->net_dev)
+ dev_put(trigger_data->net_dev);
+
+ dev_hold(old_net);
+ trigger_data->net_dev = old_net;
+ memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
+
+ spin_unlock_bh(&trigger_data->lock);
+ return -EINVAL;
+ }
+
trigger_data->carrier_link_up = false;
if (trigger_data->net_dev != NULL)
trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
@@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
size_t size, enum led_trigger_netdev_modes attr)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
- unsigned long state;
+ unsigned long state, old_mode = trigger_data->mode;
int ret;
int bit;
@@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
else
clear_bit(bit, &trigger_data->mode);
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old mode on validation fail */
+ trigger_data->mode = old_mode;
+ return -EINVAL;
+ }
+
set_baseline_state(trigger_data);
return size;
@@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
size_t size)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ int old_interval = atomic_read(&trigger_data->interval);
+ u32 old_mode = trigger_data->mode;
unsigned long value;
int ret;
@@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
return ret;
/* impose some basic bounds on the timer interval */
- if (value >= 5 && value <= 10000) {
- cancel_delayed_work_sync(&trigger_data->work);
+ if (value < 5 || value > 10000)
+ return -EINVAL;
+
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
- atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
- set_baseline_state(trigger_data); /* resets timer */
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old interval on validation error */
+ atomic_set(&trigger_data->interval, old_interval);
+ trigger_data->mode = old_mode;
+ return -EINVAL;
}
+ set_baseline_state(trigger_data); /* resets timer */
+
return size;
}
@@ -368,13 +498,25 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
trigger_data->mode = 0;
atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
trigger_data->last_activity = 0;
+ if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
+ /* With hw mode enabled reset any rule set by default */
+ if (led_cdev->hw_control_status(led_cdev)) {
+ rc = led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
+ BLINK_MODE_ZERO);
+ if (rc)
+ goto err;
+ }
+ }
led_set_trigger_data(led_cdev, trigger_data);
rc = register_netdevice_notifier(&trigger_data->notifier);
if (rc)
- kfree(trigger_data);
+ goto err;
+ return 0;
+err:
+ kfree(trigger_data);
return rc;
}
@@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
static struct led_trigger netdev_led_trigger = {
.name = "netdev",
+ .supported_blink_modes = SOFTWARE_HARDWARE,
.activate = netdev_trig_activate,
.deactivate = netdev_trig_deactivate,
.groups = netdev_trig_groups,
--
2.34.1
On Tue, 03 May 2022 17:16:33 +0200, Ansuel Smith wrote:
> Add LEDs definition example for qca8k using the offload trigger as the
> default trigger and add all the supported offload triggers by the
> switch.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/net/dsa/qca8k.example.dts:209.42-43 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/net/dsa/qca8k.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1401: dt_binding_check] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
On Tue, May 03, 2022 at 05:16:33PM +0200, Ansuel Smith wrote:
> Add LEDs definition example for qca8k using the offload trigger as the
> default trigger and add all the supported offload triggers by the
> switch.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index f3c88371d76c..9b46ef645a2d 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -65,6 +65,8 @@ properties:
> internal mdio access is used.
> With the legacy mapping the reg corresponding to the internal
> mdio is the switch reg with an offset of -1.
> + Each phy have at least 3 LEDs connected and can be declared
s/at least/up to/ ?
Or your example is wrong with only 2.
> + using the standard LEDs structure.
>
> patternProperties:
> "^(ethernet-)?ports$":
> @@ -287,6 +289,24 @@ examples:
>
> internal_phy_port1: ethernet-phy@0 {
> reg = <0>;
> +
> + leds {
> + led@0 {
> + reg = <0>;
> + color = <LED_COLOR_ID_WHITE>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + linux,default-trigger = "netdev";
> + };
> +
> + led@1 {
> + reg = <1>;
> + color = <LED_COLOR_ID_AMBER>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + linux,default-trigger = "netdev";
> + };
> + };
> };
>
> internal_phy_port2: ethernet-phy@1 {
> --
> 2.34.1
>
>
> +struct netdev_led_attr_detail {
> + char *name;
> + bool hardware_only;
> + enum led_trigger_netdev_modes bit;
> +};
> +
> +static struct netdev_led_attr_detail attr_details[] = {
> + { .name = "link", .bit = TRIGGER_NETDEV_LINK},
> + { .name = "tx", .bit = TRIGGER_NETDEV_TX},
> + { .name = "rx", .bit = TRIGGER_NETDEV_RX},
hardware_only is never set. Maybe it is used in a later patch? If so,
please introduce it there.
> static void set_baseline_state(struct led_netdev_data *trigger_data)
> {
> + int i;
> int current_brightness;
> + struct netdev_led_attr_detail *detail;
> struct led_classdev *led_cdev = trigger_data->led_cdev;
This file mostly keeps with reverse christmas tree, probably because
it was written by a netdev developer. It is probably not required for
the LED subsystem, but it would be nice to keep the file consistent.
> @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> size_t size)
> {
> struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> + struct net_device *old_net = trigger_data->net_dev;
> + char old_device_name[IFNAMSIZ];
>
> if (size >= IFNAMSIZ)
> return -EINVAL;
>
> + /* Backup old device name */
> + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> +
> cancel_delayed_work_sync(&trigger_data->work);
>
> spin_lock_bh(&trigger_data->lock);
> @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> trigger_data->net_dev =
> dev_get_by_name(&init_net, trigger_data->device_name);
>
> + if (!validate_baseline_state(trigger_data)) {
You probably want to validate trigger_data->net_dev is not NULL first. The current code
is a little odd with that,
> + /* Restore old net_dev and device_name */
> + if (trigger_data->net_dev)
> + dev_put(trigger_data->net_dev);
> +
> + dev_hold(old_net);
This dev_hold() looks wrong. It is trying to undo a dev_put()
somewhere? You should not actually do a put until you know you really
do not old_net, otherwise there is a danger it disappears and you
cannot undo.
> @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> return ret;
>
> /* impose some basic bounds on the timer interval */
> - if (value >= 5 && value <= 10000) {
> - cancel_delayed_work_sync(&trigger_data->work);
> + if (value < 5 || value > 10000)
> + return -EINVAL;
> +
> + cancel_delayed_work_sync(&trigger_data->work);
> +
> + atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
>
> - atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> - set_baseline_state(trigger_data); /* resets timer */
> + if (!validate_baseline_state(trigger_data)) {
> + /* Restore old interval on validation error */
> + atomic_set(&trigger_data->interval, old_interval);
> + trigger_data->mode = old_mode;
I think you need to schedule the work again, since you cancelled
it. It is at the end of the work that the next work is scheduled, and
so it will not self recover.
Andrew
On Thu, May 05, 2022 at 01:25:31AM +0200, Andrew Lunn wrote:
> On Tue, May 03, 2022 at 05:16:25PM +0200, Ansuel Smith wrote:
> > Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
> > that will be true or false based on the carrier link. No functional
> > change intended.
>
> What is missing from the commit message is an explanation why?
>
> Andrew
Will add the reason.
Just in case it doesn't make sense...
The reason is that putting a state in the mode bitmap doesn't look
correct. It's ""acceptable"" if we have only 3 state (rx, tx and link).
It become problematic when we start to have 7 modes and a link up state
should be handled differently.
Does it make sense?
--
Ansuel
On Thu, May 05, 2022 at 03:00:03AM +0200, Andrew Lunn wrote:
> > +struct netdev_led_attr_detail {
> > + char *name;
> > + bool hardware_only;
> > + enum led_trigger_netdev_modes bit;
> > +};
> > +
> > +static struct netdev_led_attr_detail attr_details[] = {
> > + { .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > + { .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > + { .name = "rx", .bit = TRIGGER_NETDEV_RX},
>
> hardware_only is never set. Maybe it is used in a later patch? If so,
> please introduce it there.
>
Is it better to introduce the hardware_only bool in the patch where the
additional "hardware only" modes are added?
> > static void set_baseline_state(struct led_netdev_data *trigger_data)
> > {
> > + int i;
> > int current_brightness;
> > + struct netdev_led_attr_detail *detail;
> > struct led_classdev *led_cdev = trigger_data->led_cdev;
>
> This file mostly keeps with reverse christmas tree, probably because
> it was written by a netdev developer. It is probably not required for
> the LED subsystem, but it would be nice to keep the file consistent.
>
The order is a bit mixed as you notice. Ok will stick to reverse
christmas.
> > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> > size_t size)
> > {
> > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > + struct net_device *old_net = trigger_data->net_dev;
> > + char old_device_name[IFNAMSIZ];
> >
> > if (size >= IFNAMSIZ)
> > return -EINVAL;
> >
> > + /* Backup old device name */
> > + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > +
> > cancel_delayed_work_sync(&trigger_data->work);
> >
> > spin_lock_bh(&trigger_data->lock);
> > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> > trigger_data->net_dev =
> > dev_get_by_name(&init_net, trigger_data->device_name);
> >
> > + if (!validate_baseline_state(trigger_data)) {
>
> You probably want to validate trigger_data->net_dev is not NULL first. The current code
> is a little odd with that,
>
The thing is that net_dev can be NULL and actually is a requirement for
hardware_mode to be triggered. (net_dev must be NULL or software mode is
forced)
> > + /* Restore old net_dev and device_name */
> > + if (trigger_data->net_dev)
> > + dev_put(trigger_data->net_dev);
> > +
> > + dev_hold(old_net);
>
> This dev_hold() looks wrong. It is trying to undo a dev_put()
> somewhere? You should not actually do a put until you know you really
> do not old_net, otherwise there is a danger it disappears and you
> cannot undo.
>
Yes if you notice some lines above, the first thing done is to dev_put
the current net_dev set. So on validation fail we restore the old state
with holding the old_net again and restoring the device_name.
But thanks for poiting it out... I should check if old_net is not NULL.
Also should i change the logic and just dev_put if all goes well? (for
example before the return size?) That way I should be able to skip this
additional dev_hold.
> > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> > return ret;
> >
> > /* impose some basic bounds on the timer interval */
> > - if (value >= 5 && value <= 10000) {
> > - cancel_delayed_work_sync(&trigger_data->work);
> > + if (value < 5 || value > 10000)
> > + return -EINVAL;
> > +
> > + cancel_delayed_work_sync(&trigger_data->work);
> > +
> > + atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> >
> > - atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> > - set_baseline_state(trigger_data); /* resets timer */
> > + if (!validate_baseline_state(trigger_data)) {
> > + /* Restore old interval on validation error */
> > + atomic_set(&trigger_data->interval, old_interval);
> > + trigger_data->mode = old_mode;
>
> I think you need to schedule the work again, since you cancelled
> it. It is at the end of the work that the next work is scheduled, and
> so it will not self recover.
>
Ok I assume the correct way to handle this is to return error and still
use the set_baseline_state... Or Also move the validate_baseline_state
up before the cancel_delayed_work_sync. But considering we require
atomic_set for the validation to work I think the right way is to
set_baseline_state even with errors (as it will reschedule the work)
> Andrew
--
Ansuel
On Wed, May 04, 2022 at 12:15:48PM -0500, Rob Herring wrote:
> On Tue, May 03, 2022 at 05:16:33PM +0200, Ansuel Smith wrote:
> > Add LEDs definition example for qca8k using the offload trigger as the
> > default trigger and add all the supported offload triggers by the
> > switch.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > .../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > index f3c88371d76c..9b46ef645a2d 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > @@ -65,6 +65,8 @@ properties:
> > internal mdio access is used.
> > With the legacy mapping the reg corresponding to the internal
> > mdio is the switch reg with an offset of -1.
> > + Each phy have at least 3 LEDs connected and can be declared
>
> s/at least/up to/ ?
>
> Or your example is wrong with only 2.
>
Up to. Internally the regs are there but 99% of the times OEM just
connect 2 of 3 LEDs. Will fix.
> > + using the standard LEDs structure.
> >
> > patternProperties:
> > "^(ethernet-)?ports$":
> > @@ -287,6 +289,24 @@ examples:
> >
> > internal_phy_port1: ethernet-phy@0 {
> > reg = <0>;
> > +
> > + leds {
> > + led@0 {
> > + reg = <0>;
> > + color = <LED_COLOR_ID_WHITE>;
> > + function = LED_FUNCTION_LAN;
> > + function-enumerator = <1>;
> > + linux,default-trigger = "netdev";
> > + };
> > +
> > + led@1 {
> > + reg = <1>;
> > + color = <LED_COLOR_ID_AMBER>;
> > + function = LED_FUNCTION_LAN;
> > + function-enumerator = <1>;
> > + linux,default-trigger = "netdev";
> > + };
> > + };
> > };
> >
> > internal_phy_port2: ethernet-phy@1 {
> > --
> > 2.34.1
> >
> >
--
Ansuel
On Tue, May 03, 2022 at 05:16:25PM +0200, Ansuel Smith wrote:
> Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
> that will be true or false based on the carrier link. No functional
> change intended.
What is missing from the commit message is an explanation why?
Andrew