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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZJL9I5rQlYFUZWPp@corigine.com>
Date: Wed, 21 Jun 2023 15:37:39 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>, Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <rmk+kernel@...linux.org.uk>,
	Christian Marangi <ansuelsmth@...il.com>
Subject: Re: [PATCH net-next v1 1/3] led: trig: netdev: Fix requesting
 offload device

On Mon, Jun 19, 2023 at 11:57:01PM +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.
> 
> Signed-off-by: Andrew Lunn <andrew@...n.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 2311dae7f070..df61977f1fa2 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -471,15 +471,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);

Hi Andrew,

Not related, and perhaps not important: I think the scope of dev could be
reduced to this block.

>  		if (dev) {
>  			const char *name = dev_name(dev);

More related, but also not important: I think the scope of mode could
be reduced tho this block.

>  
>  			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);
> +			if (!rc)
> +				trigger_data->mode = mode;

Is the case where trigger_data->hw_control is set to true
but trigger_data->mode is not set ok?

I understand that is the whole point is not to return an error in this case.
But I'm concerned about the value of trigger_data->mode.

Probably it is ok.
But I feel that it is worth asking.

>  		}
>  	}
>  
> -- 
> 2.40.1
>