| Message ID | 20230624205629.4158216-2-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support offload LED blinking to PHY. | expand |
On Sat, Jun 24, 2023 at 10:56:27PM +0200, Andrew Lunn wrote: > When the netdev trigger is activates, it tries to determine what > device the LED blinks for, and what the current blink mode is. > > The documentation for hw_control_get() says: > > * Return 0 on success, a negative error number on failing parsing the > * initial mode. Error from this function is NOT FATAL as the device > * may be in a not supported initial state by the attached LED trigger. > */ > > For the Marvell PHY and the Armada 370-rd board, the initial LED blink > mode is not supported by the trigger, so it returns an error. This > resulted in not getting the device the LED is blinking for. As a > result, the device is unknown and offloaded is never performed. > > Change to condition to always get the device if offloading is > supported, and reduce the scope of testing for an error from > hw_control_get() to skip setting trigger internal state if there is an > error. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/leds/trigger/ledtrig-netdev.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c > index 32b66703068a..247b100ee1d3 100644 > --- a/drivers/leds/trigger/ledtrig-netdev.c > +++ b/drivers/leds/trigger/ledtrig-netdev.c > @@ -565,15 +565,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev) > /* Check if hw control is active by default on the LED. > * Init already enabled mode in hw control. > */ > - if (supports_hw_control(led_cdev) && > - !led_cdev->hw_control_get(led_cdev, &mode)) { > + if (supports_hw_control(led_cdev)) { > dev = led_cdev->hw_control_get_device(led_cdev); > if (dev) { > const char *name = dev_name(dev); > > set_device_name(trigger_data, name, strlen(name)); > trigger_data->hw_control = true; > - trigger_data->mode = mode; > + > + rc = led_cdev->hw_control_get(led_cdev, &mode); Shouldn't there also be something like led_cdev->hw_control_get(led_cdev, 0); in netdev_trig_deactivate then? Because somehow we need to tell the hardware to no longer perform an offloading operation. > + if (!rc) > + trigger_data->mode = mode; > } > } > > -- > 2.40.1 > >
> > + if (supports_hw_control(led_cdev)) { > > dev = led_cdev->hw_control_get_device(led_cdev); > > if (dev) { > > const char *name = dev_name(dev); > > > > set_device_name(trigger_data, name, strlen(name)); > > trigger_data->hw_control = true; > > - trigger_data->mode = mode; > > + > > + rc = led_cdev->hw_control_get(led_cdev, &mode); > > Shouldn't there also be something like > led_cdev->hw_control_get(led_cdev, 0); > in netdev_trig_deactivate then? > Because somehow we need to tell the hardware to no longer perform an > offloading operation. Hi Daniel Back from vacation, so getting around to this now. Interesting question. I would actually expect the trigger that takes its place will set the brightness to what it wants it to default it. It is documented that setting the brightness disables any offload. Have you seen a real problem with changing triggers? Andrew
Hi Andrew, On Tue, Aug 08, 2023 at 12:27:10AM +0200, Andrew Lunn wrote: > > > + if (supports_hw_control(led_cdev)) { > > > dev = led_cdev->hw_control_get_device(led_cdev); > > > if (dev) { > > > const char *name = dev_name(dev); > > > > > > set_device_name(trigger_data, name, strlen(name)); > > > trigger_data->hw_control = true; > > > - trigger_data->mode = mode; > > > + > > > + rc = led_cdev->hw_control_get(led_cdev, &mode); > > > > Shouldn't there also be something like > > led_cdev->hw_control_get(led_cdev, 0); > > in netdev_trig_deactivate then? > > Because somehow we need to tell the hardware to no longer perform an > > offloading operation. > > Hi Daniel > > Back from vacation, so getting around to this now. > > Interesting question. I would actually expect the trigger that takes > its place will set the brightness to what it wants it to default > it. It is documented that setting the brightness disables any offload. So setting the brigthness should result in the trigger to be cleared back to 'none' then, and that would result in calling netdev_trig_deactivate if it was previously active. Because otherwise, even if I take care of truning off all hardware triggers in the led_set_brightness call, the netdev trigger would still be selected. > > Have you seen a real problem with changing triggers? Yes, when manually switching from the netdev trigger to none (or any other trigger), hardware offloading would remain active with my implementation of the PHY LED driver[1] (which doesn't clear any offloading related things but only sets/clears a FORCE_ON bit in its led_set_brightness function). [1]: https://github.com/dangowrt/linux/commit/439d52d7b80c97ff0c682ec68a70812030c3d79e Cheers Daniel
> So setting the brigthness should result in the trigger to be cleared > back to 'none' then, and that would result in calling > netdev_trig_deactivate if it was previously active. > > Because otherwise, even if I take care of truning off all hardware > triggers in the led_set_brightness call, the netdev trigger would > still be selected. I looked at edtrig-timer.c, which can offload the blinking to hardware if it supports the needed op. Its deactivate function is: static void timer_trig_deactivate(struct led_classdev *led_cdev) { /* Stop blinking */ led_set_brightness(led_cdev, LED_OFF); } So this does suggest the trigger should disable offload. v3 will do similar. Andrew
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index 32b66703068a..247b100ee1d3 100644 --- a/drivers/leds/trigger/ledtrig-netdev.c +++ b/drivers/leds/trigger/ledtrig-netdev.c @@ -565,15 +565,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev) /* Check if hw control is active by default on the LED. * Init already enabled mode in hw control. */ - if (supports_hw_control(led_cdev) && - !led_cdev->hw_control_get(led_cdev, &mode)) { + if (supports_hw_control(led_cdev)) { dev = led_cdev->hw_control_get_device(led_cdev); if (dev) { const char *name = dev_name(dev); set_device_name(trigger_data, name, strlen(name)); trigger_data->hw_control = true; - trigger_data->mode = mode; + + rc = led_cdev->hw_control_get(led_cdev, &mode); + if (!rc) + trigger_data->mode = mode; } }