Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Ansuel Smith]

On Mon, Nov 08, 2021 at 05:13:12PM +0100, Marek Behún wrote:
> On Mon, 8 Nov 2021 16:16:13 +0100
> Ansuel Smith <ansuelsmth@xxxxxxxxx> wrote:
> 
> > On Mon, Nov 08, 2021 at 03:04:23PM +0100, Andrew Lunn wrote:
> > > > +static inline int led_trigger_offload(struct led_classdev *led_cdev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (!led_cdev->trigger_offload)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	ret = led_cdev->trigger_offload(led_cdev, true);
> > > > +	led_cdev->offloaded = !ret;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
> > > > +{
> > > > +	if (!led_cdev->trigger_offload)
> > > > +		return;
> > > > +
> > > > +	if (led_cdev->offloaded) {
> > > > +		led_cdev->trigger_offload(led_cdev, false);
> > > > +		led_cdev->offloaded = false;
> > > > +	}
> > > > +}
> > > > +#endif  
> > > 
> > > I think there should be two calls into the cdev driver, not this
> > > true/false parameter. trigger_offload_start() and
> > > trigger_offload_stop().
> > >   
> > 
> > To not add too much function to the struct, can we introduce one
> > function that both enable and disable the hw mode?
> 
> Dear Ansuel,
>

(just to make sure, I don't want to look rude, I just want this feature
finally introduced and supported since AFAIK many tried to add support
for LEDs in PHY but everyone failed. For a reason or another, too
specific, not generic...)

> what is the purpose of adding trigger_offload() methods to LED, if you
> are not going to add support to offload the netdev trigger? That was
> the entire purpose when I wrote that patch.

But the final step was adding LEDs support for PHY. The idea was to find
a clean way by passing the idea of offloading a trigger... But fact is
that LEDs in PHY operate by themself so it would add overhead selecting
a trigger that will use event/works just to then be ignored as the
trigger is offloaded.
Also I think we are missing the fact that most of PHY can operate in
software or in hardware. NOT BOTH. So the entire concept of offloading
some trigger won't work as we were not able to simulate with SW
unsupported trigger. (not the case with netdev as it does only support
link, rx/tx but that was to explain the concept that SW and HW mode are
mutually exclusive.)

> 
> If you just want to create a new trigger that will make the PHY chip do
> the blinking, there is no need at all for the offloading patch.
> 

Again the idea here is that a LED can offer a way to run by HW and then
a trigger configure them and enables the mode. I see the offload and
configure function needed anyway side from the implementation.

> And you will also get a NACK from me and also Pavel (LED subsystem
> maintainer).
> 

Can we try to find a common way to introduce this?

> The current plan is to:
> - add support for offloading existing LED triggers to HW (LED
>   controllers (PHY chips, for example))
> - make netdev trigger try offloading itself to HW via this new API (if
>   it fails, netdev trigger will blink the LED in SW as it does now)

Can't be done. If in HW mode, we should just declare a trigger not
supported in SW if we really want to follow the netdev expanding path.
But still that would mean filling the netdev trigger with extra code and
condition that will only apply in HW mode. At this point just create a
dedicated trigger.
We can consider introducing the same sysfs used by netdev trigger but
nothing else.

> - create LED classdevices in a PHY driver that have the offload()
>   methods implemented. The offload method looks at what trigger is
>   being enabled for the LED, and it if it is a netdev trigger with such
>   settings that are possible to offload, it will be offloaded.
> 
>   This whole thing makes use of the existing sysfs ABI.
>   So for example if I do
>     cd /sys/class/net/eth0/phydev/leds/<LED>
>     echo netdev >trigger
>     echo eth0 >device_name

How would this work in HW mode? The PHY blink only with packet in his
port. We can't tell the PHY to HW blink based on an interface.
That is the main problem by using netdev for PHYs, netdev is flexible
but an offload trigger is not and would work only based on some
condition. We should hardcode the device_name in HW and again pollute
the netdev trigger more.
An idea would be add tons of check to netdev on every event to check the
interface but that wouldn't go against any offload idea? The system will
be loaded anyway with all these checks.

>     echo 1 >rx
>     echo 1 >tx
>   The netdev trigger is activated, and it calls the offload() method.
>   The offload() method is implemented in the PHY driver, and it checks
>   that it can offload these settings (blink on rx/tx), and will enable
>   this.
> - extend netdev trigger to support more settings:
>   - indicate link for specific link modes only (for example 1g, 100m)
>   - ...
> - extend PHY drivers to support offloading of these new settings
> 
> Marek

The rest of the implementation is very similar. Except we just NOT use
netdev. And to me it does seems the most sane way to handle offload for
a LED. (considering the specific situation of a PHY)

>From what I can see we have 2 path:
- Pollute netdev trigger to add entire different function for HW. (that
  will end up in complex condition and more load/overhead)
- Introduce a new way to entirely offload some triggers.

Could be that here I'm just using the wrong word and I should use
hardware instead offload. But considering we are offloading some trigger
(example rx/tx/link) it's not that wrong.

The thing is why trying to expand a trigger that will just remove some
flexibility when we can solve the problem at the source with some
additional API that currently we lack any support (leds can be
configured to run by hw) and some dedicated trigger that will do the
task in a cleaner way (and without adding extra load/overhead by really
offloading the task) ?

Again hope I didn't seem rude in this message but I just want to find a
solution and proposing a new idea/explaining my concern.

-- 
	Ansuel