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]

Date: Wed, 21 Jun 2023 17:34:30 +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 Wed, Jun 21, 2023 at 05:19:01PM +0200, Andrew Lunn wrote:
> > >  			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.
> 
> Yes, its something Christian and I talked about off-list.
> trigger_data->mode is 0 by default due to the kzalloc(). 0 is a valid
> value, it means don't blink for any reason. So in effect the LED
> should be off. And any LED driver which the ledtrig-netdev.c supports
> must support software control of the LED, so does support setting the
> LED off.
> 
> In the normal case hw_control_get() returns indicating the current
> blink mode, and the trigger sets its initial state to that. If
> however, it returns an error, it probably means its current state
> cannot be represented by the netdev trigger. PHY vendors do all sort
> of odd things, and we don't want to support all the craziness. So
> setting the LED off and leaving the user to configure the LED how they
> want seems like a reasonable thing to do.
> 
> And i tested this because my initial implementation of the Marvell
> driver was FUBAR and it returned an error here.

Thanks Andrew,

sounds good to me.
Especially,

	"we don't want to support all the craziness"

Reviewed-by: Simon Horman <simon.horman@...igine.com>