mbox series

[RFC,v6,00/11] Adds support for PHY LEDs with offload triggers

Message ID 20220503151633.18760-1-ansuelsmth@gmail.com
Headers show
Series Adds support for PHY LEDs with offload triggers | expand

Message

Christian Marangi May 3, 2022, 3:16 p.m. UTC
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

Comments

Rob Herring (Arm) May 3, 2022, 10:21 p.m. UTC | #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 <ansuelsmth@gmail.com>
> ---
>  .../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.
Rob Herring (Arm) May 4, 2022, 5:15 p.m. UTC | #2
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 <ansuelsmth@gmail.com>
> ---
>  .../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
> 
>
Andrew Lunn May 4, 2022, 11:23 p.m. UTC | #3
> +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
> +requested blink mode (flags) or not.

-EOPNOTSUPP might be clearer.


> +In ZERO hw_control_configure() should return 0 with success operation or error.
> +
> +The unsigned long flag is specific to the trigger and change across them. It's in the LED
> +driver interest know how to elaborate this flag and to declare support for a
> +particular trigger. For this exact reason explicit support for the specific
> +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> +with a not supported trigger.
> +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
> +fail as the driver doesn't support that specific offload trigger or doesn't know
> +how to handle the provided flags.
> +
>  Known Issues
>  ============
>  
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 09ff1dc6f48d..b5aad67fecfb 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -73,6 +73,16 @@ enum led_blink_modes {
>  	SOFTWARE_HARDWARE_CONTROLLED,
>  };
>  
> +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> +enum blink_mode_cmd {
> +	BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> +	BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> +	BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> +	BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> +	BLINK_MODE_ZERO, /* Disable any hardware blink active */
> +};
> +#endif

Skip the #ifdef. The enum itself takes no space if never used, and it
makes the driver simpler if they always exist.

> +
>  struct led_classdev {
>  	const char		*name;
>  	unsigned int brightness;
> @@ -185,6 +195,17 @@ struct led_classdev {
>  	 * the old status but that is not mandatory and also putting it off is accepted.
>  	 */
>  	int			(*hw_control_stop)(struct led_classdev *led_cdev);
> +	/* This will be used to configure the various blink modes LED support in hardware
> +	 * mode.
> +	 * The LED driver require to support the active trigger and will elaborate the
> +	 * unsigned long flag and do the operation based on the provided cmd.
> +	 * Current operation are enable,disable,supported and status.
> +	 * A trigger will use this to enable or disable the asked blink mode, check the
> +	 * status of the blink mode or ask if the blink mode can run in hardware mode.
> +	 */
> +	int			(*hw_control_configure)(struct led_classdev *led_cdev,
> +							unsigned long flag,
> +							enum blink_mode_cmd cmd);
>  #endif
>  #endif
>  
> @@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
>  	return led_cdev->trigger_data;
>  }
>  
> +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> +static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev,
> +						       unsigned long flag)
> +{
> +	int ret;
> +
> +	/* Sanity check: make sure led support hw mode */
> +	if (led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> +		return false;
> +
> +	ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED);
> +	if (ret > 0)
> +		return true;
> +
> +	return false;
> +}
> +#endif

Please add a version which returns false when
CONFIG_LEDS_HARDWARE_CONTROL is disabled.

Does this actually need to be an inline function?

     Andrew
Andrew Lunn May 4, 2022, 11:25 p.m. UTC | #4
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
Andrew Lunn May 5, 2022, 1 a.m. UTC | #5
> +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
Andrew Lunn May 5, 2022, 1:46 a.m. UTC | #6
> +config NET_DSA_QCA8K_LEDS_SUPPORT
> +	tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> +	select NET_DSA_QCA8K

The should be a depends, not a select. It will then become visible
when the NET_DSA_QCA8K directly above it is enabled.

> +	select LEDS_OFFLOAD_TRIGGERS

and this should also be a depends. If the LED core does not have
support, the QCA8K driver should not enable its support.

> +static int
> +qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask)
> +{
> +	/* Parsing specific to netdev trigger */
> +	if (test_bit(TRIGGER_NETDEV_LINK, &rules))
> +		*offload_trigger = QCA8K_LED_LINK_10M_EN_MASK |
> +				   QCA8K_LED_LINK_100M_EN_MASK |
> +				   QCA8K_LED_LINK_1000M_EN_MASK;
> +	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
> +		*offload_trigger = QCA8K_LED_LINK_10M_EN_MASK;
> +	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
> +		*offload_trigger = QCA8K_LED_LINK_100M_EN_MASK;
> +	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
> +		*offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK;
> +	if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
> +		*offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK;
> +	if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
> +		*offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK;
> +	if (test_bit(TRIGGER_NETDEV_TX, &rules))
> +		*offload_trigger = QCA8K_LED_TX_BLINK_MASK;
> +	if (test_bit(TRIGGER_NETDEV_RX, &rules))
> +		*offload_trigger = QCA8K_LED_RX_BLINK_MASK;
> +	if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules))
> +		*offload_trigger = QCA8K_LED_BLINK_2HZ;
> +	if (test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules))
> +		*offload_trigger = QCA8K_LED_BLINK_4HZ;
> +	if (test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules))
> +		*offload_trigger = QCA8K_LED_BLINK_8HZ;
> +
> +	pr_info("OFFLOAD TRIGGER %x\n", *offload_trigger);

leftover debug print.

	 Andrew
Andrew Lunn May 5, 2022, 1:55 a.m. UTC | #7
> +		ret = fwnode_property_read_string(led, "default-state", &state);

You should probably use led_default_state led_init_default_state_get()

    Andrew
Christian Marangi May 5, 2022, 1:02 p.m. UTC | #8
On Thu, May 05, 2022 at 01:23:36AM +0200, Andrew Lunn wrote:
> > +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
> > +requested blink mode (flags) or not.
> 
> -EOPNOTSUPP might be clearer.
> 
> 
> > +In ZERO hw_control_configure() should return 0 with success operation or error.
> > +
> > +The unsigned long flag is specific to the trigger and change across them. It's in the LED
> > +driver interest know how to elaborate this flag and to declare support for a
> > +particular trigger. For this exact reason explicit support for the specific
> > +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> > +with a not supported trigger.
> > +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
> > +fail as the driver doesn't support that specific offload trigger or doesn't know
> > +how to handle the provided flags.
> > +
> >  Known Issues
> >  ============
> >  
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 09ff1dc6f48d..b5aad67fecfb 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -73,6 +73,16 @@ enum led_blink_modes {
> >  	SOFTWARE_HARDWARE_CONTROLLED,
> >  };
> >  
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +enum blink_mode_cmd {
> > +	BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> > +	BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> > +	BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> > +	BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> > +	BLINK_MODE_ZERO, /* Disable any hardware blink active */
> > +};
> > +#endif
> 
> Skip the #ifdef. The enum itself takes no space if never used, and it
> makes the driver simpler if they always exist.
> 
> > +
> >  struct led_classdev {
> >  	const char		*name;
> >  	unsigned int brightness;
> > @@ -185,6 +195,17 @@ struct led_classdev {
> >  	 * the old status but that is not mandatory and also putting it off is accepted.
> >  	 */
> >  	int			(*hw_control_stop)(struct led_classdev *led_cdev);
> > +	/* This will be used to configure the various blink modes LED support in hardware
> > +	 * mode.
> > +	 * The LED driver require to support the active trigger and will elaborate the
> > +	 * unsigned long flag and do the operation based on the provided cmd.
> > +	 * Current operation are enable,disable,supported and status.
> > +	 * A trigger will use this to enable or disable the asked blink mode, check the
> > +	 * status of the blink mode or ask if the blink mode can run in hardware mode.
> > +	 */
> > +	int			(*hw_control_configure)(struct led_classdev *led_cdev,
> > +							unsigned long flag,
> > +							enum blink_mode_cmd cmd);
> >  #endif
> >  #endif
> >  
> > @@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
> >  	return led_cdev->trigger_data;
> >  }
> >  
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev,
> > +						       unsigned long flag)
> > +{
> > +	int ret;
> > +
> > +	/* Sanity check: make sure led support hw mode */
> > +	if (led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> > +		return false;
> > +
> > +	ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED);
> > +	if (ret > 0)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +#endif
> 
> Please add a version which returns false when
> CONFIG_LEDS_HARDWARE_CONTROL is disabled.
> 
> Does this actually need to be an inline function?

Not at all... Originally it was a smaller function. Will drop it.

>
>      Andrew
Christian Marangi May 5, 2022, 1:05 p.m. UTC | #9
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?
Christian Marangi May 5, 2022, 1:27 p.m. UTC | #10
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
Christian Marangi May 5, 2022, 1:33 p.m. UTC | #11
On Thu, May 05, 2022 at 03:55:26AM +0200, Andrew Lunn wrote:
> > +		ret = fwnode_property_read_string(led, "default-state", &state);
> 
> You should probably use led_default_state led_init_default_state_get()
> 
>     Andrew

Oh, didn't know it was a thing, is this new? Anyway thanks.
Christian Marangi May 5, 2022, 1:34 p.m. UTC | #12
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 <ansuelsmth@gmail.com>
> > ---
> >  .../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
> > 
> >
Andrew Lunn May 5, 2022, 2:24 p.m. UTC | #13
On Thu, May 05, 2022 at 03:33:07PM +0200, Ansuel Smith wrote:
> On Thu, May 05, 2022 at 03:55:26AM +0200, Andrew Lunn wrote:
> > > +		ret = fwnode_property_read_string(led, "default-state", &state);
> > 
> > You should probably use led_default_state led_init_default_state_get()
> > 
> >     Andrew
> 
> Oh, didn't know it was a thing, is this new? Anyway thanks.

No idea. But my thinking was, you cannot be the first to implement the
binding, there probably exists some helpers somewhere...

General rule of thumb: Assume somebody has already been there and done
it, you just need to find it and reuse it.

    Andrew