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 for Android: free password hash cracker in your pocket

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

Date: Wed, 21 Jun 2023 17:19:01 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Simon Horman <simon.horman@...igine.com>
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

> >  			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.

     Andrew