Re: [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device

[Daniel Golle <daniel@makrotopia.org>]

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