netdev - Re: [PATCH] leds: trigger: netdev: skip setting baseline state in activate if hw-controlled

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: Tue, 5 Dec 2023 13:40:32 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
 Christian Marangi <ansuelsmth@...il.com>, Jakub Kicinski <kuba@...nel.org>,
 "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] leds: trigger: netdev: skip setting baseline state in
 activate if hw-controlled

On 05.12.2023 04:00, Andrew Lunn wrote:
>> Let's take a very simple use case: We have a one bit configuration to
>> switch a LED between link_100 and link_1000 hw trigger mode.
>>
>> Then we have the atomicity issue you described: We can't go directly
>> from one hw-controlled mode to the other, we have to go via both
>> modes active or no mode active.
>>
>> And unfortunately we don't have the option to indicate this by some
>> optical LED activity like blinking, especially if the link is down
>> at the moment.
>>
>> Would be a pity if our nice framework can't support such a simple
>> use case. So, what I could imagine, we react based on the return code
>> from hw_control_is_supported():
>>
>> - 0: use hw control
>> - -EOPNOTSUPP: fall back to LED software control, no error returned to use
>> - -ENOTSUPP (another idea: ENOEXEC): store new mode in trigger_data->mode and return error to the user
>> - other errors: don't store new mode and return error to user
>>
>> Not fully intuitive and the subtle difference between EOPNOTSUPP and
>> ENOTSUPP may confuse driver authors adding device LED support.
> 
> Using an NFS error code for LEDs will definitely confuse
> developers. This is not a network file system, where it is valid to
> use ENOTSUPP.
> 
> I actually think we need to define some best practices, ordered on
> what the hardware can do.
> 
> 1) With software control, set_brightness should do what you expect,
> not return an error.
> 
> 2) Without full software control, but there is a mechanism to report a
> problem, like constant blinking, or off, do that, and return
> -EOPNOTSUPP.
> 
> 3) Really dumb hardware like this, set_brightness should be a NULL
> pointer. The core returns -EOPNOTSUPP.
> 
> The core should return this -EOPNOTSUPP to user space, but it should
> accept the configuration change. So the user can put it into an
> invalid state, in order to get to a valid state with further
> configuration.
> 
Sounds good to me. Let me come up with a RFC patch.

> I don't see an easy way to let the user know what the valid states
> are. We currently have a 10bit state. I don't think we can put all the
> valid ones in a /sysfs file, especially when QCA8K pretty much
> supports everything.
> 
> 	 Andrew

Heiner