Re: [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device

[Daniel Golle <daniel@makrotopia.org>]

On Sat, Jun 24, 2023 at 10:56:27PM +0200, Andrew Lunn wrote:
> When the netdev trigger is activates, it tries to determine what
> device the LED blinks for, and what the current blink mode is.
> 
> The documentation for hw_control_get() says:
> 
> 	 * Return 0 on success, a negative error number on failing parsing the
> 	 * initial mode. Error from this function is NOT FATAL as the device
> 	 * may be in a not supported initial state by the attached LED trigger.
> 	 */
> 
> For the Marvell PHY and the Armada 370-rd board, the initial LED blink
> mode is not supported by the trigger, so it returns an error. This
> resulted in not getting the device the LED is blinking for. As a
> result, the device is unknown and offloaded is never performed.
> 
> Change to condition to always get the device if offloading is
> supported, and reduce the scope of testing for an error from
> hw_control_get() to skip setting trigger internal state if there is an
> error.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 32b66703068a..247b100ee1d3 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -565,15 +565,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>  	/* Check if hw control is active by default on the LED.
>  	 * Init already enabled mode in hw control.
>  	 */
> -	if (supports_hw_control(led_cdev) &&
> -	    !led_cdev->hw_control_get(led_cdev, &mode)) {
> +	if (supports_hw_control(led_cdev)) {
>  		dev = led_cdev->hw_control_get_device(led_cdev);
>  		if (dev) {
>  			const char *name = dev_name(dev);
>  
>  			set_device_name(trigger_data, name, strlen(name));
>  			trigger_data->hw_control = true;
> -			trigger_data->mode = mode;
> +
> +			rc = led_cdev->hw_control_get(led_cdev, &mode);

Shouldn't there also be something like
led_cdev->hw_control_get(led_cdev, 0);
in netdev_trig_deactivate then?
Because somehow we need to tell the hardware to no longer perform an
offloading operation.

> +			if (!rc)
> +				trigger_data->mode = mode;
>  		}
>  	}
>  
> -- 
> 2.40.1
> 
>