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

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

[Heiner Kallweit]

The current codes uses the sw_control path in set_baseline_state() when
called from netdev_trig_activate() even if we're hw-controlled. This
may result in errors when led_set_brightness() is called because we may
not have set_brightness led ops (if hw doesn't support setting a LED
to ON).

Therefore set trigger_data->hw_control = true before calling
set_device_name() from netdev_trig_activate(). In this call chain we
have to prevent set_baseline_state() from being called, because this
would call hw_control_set(). Use led_cdev->trigger_data == NULL as
indicator for being called from netdev_trig_activate().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 7ed2d0b64..b58396600 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -251,7 +251,11 @@ static int set_device_name(struct led_netdev_data *trigger_data,
 
 	trigger_data->last_activity = 0;
 
-	set_baseline_state(trigger_data);
+	/* skip if we're called from netdev_trig_activate() and hw_control is true */
+	if (!trigger_data->hw_control ||
+	    led_get_trigger_data(trigger_data->led_cdev))
+		set_baseline_state(trigger_data);
+
 	mutex_unlock(&trigger_data->lock);
 	rtnl_unlock();
 
@@ -568,8 +572,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 		if (dev) {
 			const char *name = dev_name(dev);
 
-			set_device_name(trigger_data, name, strlen(name));
 			trigger_data->hw_control = true;
+			set_device_name(trigger_data, name, strlen(name));
 
 			rc = led_cdev->hw_control_get(led_cdev, &mode);
 			if (!rc)
-- 
2.43.0

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

[Andrew Lunn]

On Wed, Nov 29, 2023 at 11:41:51AM +0100, Heiner Kallweit wrote:
> The current codes uses the sw_control path in set_baseline_state() when
> called from netdev_trig_activate() even if we're hw-controlled. This
> may result in errors when led_set_brightness() is called because we may
> not have set_brightness led ops (if hw doesn't support setting a LED
> to ON).

Not having software on/off control of the LED is a problem. It breaks
the whole concept of offloading/accelerating. If we cannot control the
LED, there is nothing to accelerate. What do we do when the user
selects a configuration which is not supported by the hardware? The
API is not atomic, you cannot set multiple things at once. So the user
might be trying to get from one offloadable configuration to another
offloadable configuration, but needs to go via an configuration which
is not offloadable. Do we return -EOPNOTSUPP?

Before we accept patches like this, we need to discuss the concept of
how we support LEDs which cannot be controlled in software.

    Andrew

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

[Heiner Kallweit]

On Wed, Nov 29, 2023 at 5:36 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Nov 29, 2023 at 11:41:51AM +0100, Heiner Kallweit wrote:
> > The current codes uses the sw_control path in set_baseline_state() when
> > called from netdev_trig_activate() even if we're hw-controlled. This
> > may result in errors when led_set_brightness() is called because we may
> > not have set_brightness led ops (if hw doesn't support setting a LED
> > to ON).
>
> Not having software on/off control of the LED is a problem. It breaks
> the whole concept of offloading/accelerating. If we cannot control the
> LED, there is nothing to accelerate. What do we do when the user
> selects a configuration which is not supported by the hardware? The
> API is not atomic, you cannot set multiple things at once. So the user
> might be trying to get from one offloadable configuration to another
> offloadable configuration, but needs to go via an configuration which
> is not offloadable. Do we return -EOPNOTSUPP?
>
The point you raise with the non-atomic API is completely valid,
however I think it's
not directly related to this patch. Here it's about a validated hw-control path.
So I think the patch is still valid.

> Before we accept patches like this, we need to discuss the concept of
> how we support LEDs which cannot be controlled in software.
>
RTL8168 LED control allows to switch between different hw triggers. I
was under the
assumption that this is not uncommon.
In order to deal with the non-atomic issue we have to set
trigger_data->mode to the
resulting new mode, based on what the user set. Question is what we show to the
user. If we show nothing, then he will expect the new mode to be active.
If we show an error, then he may assume that his input had no effect.
So we may have to show technically an OK, plus a message that his input has been
stored, but is not supported by hw.








>     Andrew

Heiner

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

[Andrew Lunn]

> RTL8168 LED control allows to switch between different hw triggers. I
> was under the
> assumption that this is not uncommon.

I did take a look around various datasheets, and i did find a couple
like this, but the majority have the ability to do software control.

> In order to deal with the non-atomic issue we have to set
> trigger_data->mode to the
> resulting new mode, based on what the user set. Question is what we show to the
> user. If we show nothing, then he will expect the new mode to be active.
> If we show an error, then he may assume that his input had no effect.
> So we may have to show technically an OK, plus a message that his input has been
> stored, but is not supported by hw.

It is hard to show anything to the user. We are just doing

echo 1 > file.

There is no channel to the user other than an error code.

There is also the question about what the LED should show. Ideally it
should indicate some sort of state to indicate there is an
error. Either constantly blink, turn off, etc. Maybe that is the
solution. You implement a set_brightness function which just indicates
an error on the LED, but otherwise return O.K?

   Andrew

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

[Heiner Kallweit]

On 29.11.2023 23:02, Andrew Lunn wrote:
>> RTL8168 LED control allows to switch between different hw triggers. I
>> was under the
>> assumption that this is not uncommon.
> 
> I did take a look around various datasheets, and i did find a couple
> like this, but the majority have the ability to do software control.
> 
>> In order to deal with the non-atomic issue we have to set
>> trigger_data->mode to the
>> resulting new mode, based on what the user set. Question is what we show to the
>> user. If we show nothing, then he will expect the new mode to be active.
>> If we show an error, then he may assume that his input had no effect.
>> So we may have to show technically an OK, plus a message that his input has been
>> stored, but is not supported by hw.
> 
> It is hard to show anything to the user. We are just doing
> 
> echo 1 > file.
> 
> There is no channel to the user other than an error code.
> 
> There is also the question about what the LED should show. Ideally it
> should indicate some sort of state to indicate there is an
> error. Either constantly blink, turn off, etc. Maybe that is the
> solution. You implement a set_brightness function which just indicates
> an error on the LED, but otherwise return O.K?
> 
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.
However for the user it should be ok, he always has the option to read
back the attribute in order to check whether new mode has been stored.
That's the best I can see so far.

>    Andrew

Heiner

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

[Andrew Lunn]

> 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

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

[Heiner Kallweit]

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

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

[Heiner Kallweit]

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

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

[Andrew Lunn]

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

That is simpler than i expected.

But i think we need to document our expectations. What do we expect an
LED driver to do when it cannot support software blinking. So please
could you add a comment somewhere. Maybe extend the

/*
 *Configurable sysfs attributes:
 *

section?

Thanks
	Andrew

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

[Heiner Kallweit]

On 06.12.2023 18:45, Andrew Lunn wrote:
>>>> 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?
> 
> That is simpler than i expected.
> 
> But i think we need to document our expectations. What do we expect an
> LED driver to do when it cannot support software blinking. So please
> could you add a comment somewhere. Maybe extend the
> 
> /*
>  *Configurable sysfs attributes:
>  *
> 
> section?
> 
Yes, this is a good place IMO. I'll submit a patch including the functional
change and the extended comment.

> Thanks
> 	Andrew

Heiner