LKML Archive on
help / color / mirror / Atom feed
From: "Marek Behún" <>
To: Ansuel Smith <>
Cc: Andrew Lunn <>,
	Vivien Didelot <>,
	Florian Fainelli <>,
	Vladimir Oltean <>,
	"David S. Miller" <>,
	Jakub Kicinski <>,
	Rob Herring <>,
	Jonathan Corbet <>, Pavel Machek <>,
	John Crispin <>,,,,,
Subject: Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers
Date: Mon, 8 Nov 2021 19:41:42 +0100	[thread overview]
Message-ID: <20211108194142.58630e60@thinkpad> (raw)
In-Reply-To: <YYllTn9W5tZLmVN8@Ansuel-xps.localdomain>

On Mon, 8 Nov 2021 18:58:38 +0100
Ansuel Smith <> wrote:

> Are you aware of any device that can have some trigger offloaded and
> still have the led triggered manually?

I don't understand why we would need such a thing.

Look, just to make it clear via an example: I have a device with a
Marvell PHY chip inside. There is a LED connected to one of the PHY LED

Marvell PHY has LED[0] control register, which supports the following
  LED is OFF
  LED is ON
  LED is ON when Link is up
  LED blinks on RX activity
  LED blinks on TX activity
  LED blinks on RX/TX activity
  LED is ON and blinks on RX/TX activity

I have code that exports this LED as a LED classdev

When I activate netdev trigger on this LED, the netdev trigger currently
just blinks the LED in software, by calling the .brightness_set()
method, which configures LED[0] control register to one of the first
two modes above (LED is OFF, LED is ON).

But I have also another patch that adds support to offloading netdev
trigger upon offloadable settings. The netdev trigger code calls the
.trigger_offload() method, which is implemented in PHY driver. This
method checks whether it is a netdev trigger that is to be offloaded,
and whether device_name is the name of the device attached to the PHY,
and then chooses one of the modes above, according to netdev trigger

So when I request netdev trigger for eth0, to indicate link and blink
on activity, the netdev trigger doesn't do anything in software. It
just calls the offload method ONCE (at the moment I am changing netdev
trigger settings). The blinking is then done by the PHY chip. Netdev
trigger doesn't do anything, at least not until I change the settings

> Talking about mixed mode, so HW and SW.

What exactly do you mean by mixed mode? There is no mixed mode.

> Asking to understand as currently the only way to impement all
> of this in netdev trigger is that:
> IF any hw offload trigger is supported (and enabled) then the entire
> netdev trigger can't work as it won't be able to simulate missing
> trigger in SW. And that would leave some flexibility.

What do you mean by missing trigger here? I think we need to clarify
what we mean by the word "trigger". Are you talking about the various
blinking modes that the PHY supports? If so, please let's call them HW
control modes, and not triggers. By "triggers" I understand triggers
that can be enabled on a LED via /sys/class/leds/<LED>/trigger.

> We need to understand how to operate in this condition. Should netdev
> detect that and ""hide"" the sysfs triggers? Should we report error?

So if I understand you correctly, you are asking about what should we
do if user asked for netdev trigger settings (currently only link, rx,
tx, interval) that can't be offloaded to the PHY chip.

Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
words "full control by SW", or ability to implement brightness_set()
method), then netdev trigger should blink the LED in SW via this
mechanism (which is something it would do now). A new sysfs file,
"offloaded", can indicate whether the trigger is offloaded to HW or not.

If, on the other hand, the LED cannot be controlled by SW, and it only
support some HW control modes, then there are multiple ways how to
implement what should be done, and we need to discuss this.

For example suppose that the PHY LED pin supports indicating LINK,
blinking on activity, or both, but it doesn't support blinking on rx
only, or tx only.

Since the LED is always indicating something about one network device,
the netdev trigger should be always activated for this LED and it
should be impossible to deactivate it. Also, it should be impossible to
change device_name.

  $ cd /sys/class/leds/<LED>
  $ cat device_name
  $ echo eth1 >device_name
  Operation not supported.
  $ echo none >trigger
  Operation not supported.

Now suppose that the driver by default enabled link indication, so we
  $ cat link
  $ cat rx
  $ cat tx

We want to enable blink on activity, but the LED supports only blinking
on both rx/tx activity, rx only or tx only is not supported.

Currently the only way to enable this is to do
  $ echo 1 >rx
  $ echo 1 >tx
but the first call asks for (link=1, rx=1, tx=0), which is impossible.

There are multiple things which can be done:
- "echo 1 >rx" indicates error, but remembers the setting
- "echo 1 >rx" quietly fails, without error indication. Something can
  be written to dmesg about nonsupported mode
- "echo 1 >rx" succeeds, but also sets tx=1
- rx and tx are non-writable, writing always fails. Another sysfs file
  is created, which lists modes that are actually supported, and allows
  to select between them. When a mode is selected, link,rx,tx are
  filled automatically, so that user may read them to know what the LED
  is actually doing
- something different?


  reply	other threads:[~2021-11-08 18:41 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-08  0:24 [RFC PATCH v2 0/5] Adds support for PHY LEDs with offload triggers Ansuel Smith
2021-11-08  0:24 ` [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers Ansuel Smith
2021-11-08  2:29   ` Randy Dunlap
2021-11-08 14:04   ` Andrew Lunn
2021-11-08 15:16     ` Ansuel Smith
2021-11-08 16:13       ` Marek Behún
2021-11-08 16:46         ` Ansuel Smith
2021-11-08 17:35           ` Marek Behún
2021-11-08 17:58             ` Ansuel Smith
2021-11-08 18:41               ` Marek Behún [this message]
2021-11-08 19:08                 ` Ansuel Smith
2021-11-08 19:17                   ` Marek Behún
2021-11-08 17:46         ` Andrew Lunn
2021-11-08 17:56           ` Marek Behún
2021-11-08 19:53             ` Andrew Lunn
2021-11-08 20:03               ` Vladimir Oltean
2021-11-08 20:11               ` Marek Behún
2021-11-08 20:15                 ` Ansuel Smith
2021-11-08 17:37       ` Andrew Lunn
2021-11-08  0:24 ` [RFC PATCH v2 2/5] leds: add function to configure offload leds Ansuel Smith
2021-11-08  2:22   ` Randy Dunlap
2021-11-08  0:24 ` [RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity trigger Ansuel Smith
2021-11-08  2:24   ` Randy Dunlap
2021-11-08 14:17   ` Andrew Lunn
2021-11-08 15:19     ` Ansuel Smith
2021-11-08  0:24 ` [RFC PATCH v2 4/5] net: dsa: qca8k: add LEDs support Ansuel Smith
2021-11-08  2:26   ` Randy Dunlap
2021-11-08  0:25 ` [RFC PATCH v2 5/5] dt-bindings: net: dsa: qca8k: add LEDs definition example Ansuel Smith

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211108194142.58630e60@thinkpad \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).