| Message ID | 20230619215703.4038619-2-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support offload LED blinking to PHY. | expand |
On Mon, Jun 19, 2023 at 11:57:01PM +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. > > 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 2311dae7f070..df61977f1fa2 100644 > --- a/drivers/leds/trigger/ledtrig-netdev.c > +++ b/drivers/leds/trigger/ledtrig-netdev.c > @@ -471,15 +471,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); Hi Andrew, Not related, and perhaps not important: I think the scope of dev could be reduced to this block. > if (dev) { > const char *name = dev_name(dev); More related, but also not important: I think the scope of mode could be reduced tho this block. > > 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; Is the case where trigger_data->hw_control is set to true but trigger_data->mode is not set ok? I understand that is the whole point is not to return an error in this case. But I'm concerned about the value of trigger_data->mode. Probably it is ok. But I feel that it is worth asking. > } > } > > -- > 2.40.1 >
> > 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; > > Is the case where trigger_data->hw_control is set to true > but trigger_data->mode is not set ok? > > I understand that is the whole point is not to return an error in this case. > But I'm concerned about the value of trigger_data->mode. Yes, its something Christian and I talked about off-list. trigger_data->mode is 0 by default due to the kzalloc(). 0 is a valid value, it means don't blink for any reason. So in effect the LED should be off. And any LED driver which the ledtrig-netdev.c supports must support software control of the LED, so does support setting the LED off. In the normal case hw_control_get() returns indicating the current blink mode, and the trigger sets its initial state to that. If however, it returns an error, it probably means its current state cannot be represented by the netdev trigger. PHY vendors do all sort of odd things, and we don't want to support all the craziness. So setting the LED off and leaving the user to configure the LED how they want seems like a reasonable thing to do. And i tested this because my initial implementation of the Marvell driver was FUBAR and it returned an error here. Andrew
On Wed, Jun 21, 2023 at 05:19:01PM +0200, Andrew Lunn wrote: > > > 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; > > > > Is the case where trigger_data->hw_control is set to true > > but trigger_data->mode is not set ok? > > > > I understand that is the whole point is not to return an error in this case. > > But I'm concerned about the value of trigger_data->mode. > > Yes, its something Christian and I talked about off-list. > trigger_data->mode is 0 by default due to the kzalloc(). 0 is a valid > value, it means don't blink for any reason. So in effect the LED > should be off. And any LED driver which the ledtrig-netdev.c supports > must support software control of the LED, so does support setting the > LED off. > > In the normal case hw_control_get() returns indicating the current > blink mode, and the trigger sets its initial state to that. If > however, it returns an error, it probably means its current state > cannot be represented by the netdev trigger. PHY vendors do all sort > of odd things, and we don't want to support all the craziness. So > setting the LED off and leaving the user to configure the LED how they > want seems like a reasonable thing to do. > > And i tested this because my initial implementation of the Marvell > driver was FUBAR and it returned an error here. Thanks Andrew, sounds good to me. Especially, "we don't want to support all the craziness" Reviewed-by: Simon Horman <simon.horman@corigine.com>
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index 2311dae7f070..df61977f1fa2 100644 --- a/drivers/leds/trigger/ledtrig-netdev.c +++ b/drivers/leds/trigger/ledtrig-netdev.c @@ -471,15 +471,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; } }
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. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/leds/trigger/ledtrig-netdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)