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 04:00:32 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Heiner Kallweit <hkallweit1@...il.com>
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

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

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