| Message ID | 49f1b91e-a637-4062-83c6-f851f7c80628@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | leds: trigger: netdev: skip setting baseline state in activate if hw-controlled | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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
> 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
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
> 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
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
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;
> >> 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
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
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)
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(-)