Message-ID: <d42dd05d-fc76-4040-aa15-8bbc4aa535f3@gmail.com>
Date: Tue, 5 Dec 2023 13:57:01 +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 13:40, Heiner Kallweit wrote:
> 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

Patch is so simple that I send it this way. What do you think?

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index ec0395a6b..a24f3aade 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -310,6 +310,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 				     size_t size, enum led_trigger_netdev_modes attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+	struct led_classdev *led_cdev = trigger_data->led_cdev;
 	unsigned long state, mode = trigger_data->mode;
 	int ret;
 	int bit;
@@ -349,6 +350,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	trigger_data->mode = mode;
 	trigger_data->hw_control = can_hw_control(trigger_data);
 
+	if (!led_cdev->brightness_set && !led_cdev->brightness_set_blocking &&
+	    !trigger_data->hw_control)
+		return -EOPNOTSUPP;
+
 	set_baseline_state(trigger_data);
 
 	return size;
-- 
2.43.0

Powered by blists - more mailing lists