linux-kernel - Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201125131926.4e47359a@nic.cz>
Date:   Wed, 25 Nov 2020 13:19:26 +0100
From:   Marek Behun <marek.behun@....cz>
To:     Pavel Machek <pavel@....cz>
Cc:     linux-leds@...r.kernel.org, Dan Murphy <dmurphy@...com>,
        Ondřej Jirman <megous@...ous.com>,
        Russell King <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>, linux-kernel@...r.kernel.org,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse
 `trigger-sources` from device tree

On Wed, 25 Nov 2020 11:42:42 +0100
Pavel Machek <pavel@....cz> wrote:

> Hi!
> 
> > Allow setting netdev LED trigger as default when given LED DT node has
> > the `trigger-sources` property pointing to a node corresponding to a
> > network device.
> > 
> > The specific netdev trigger mode is determined from the `function` LED
> > property.  
> 
> Sounds reasonable.
> 
> > +	netdev = of_find_net_device_by_node(args.np);
> > +	if (!netdev)
> > +		return false;
> > +
> > +	np = dev_of_node(led_cdev->dev);
> > +	if (!np)
> > +		return false;  
> 
> Missing of_node_put?

I will look into this.

> 
> > +++ b/include/dt-bindings/leds/common.h
> > @@ -77,6 +77,7 @@
> >  #define LED_FUNCTION_HEARTBEAT "heartbeat"
> >  #define LED_FUNCTION_INDICATOR "indicator"
> >  #define LED_FUNCTION_LAN "lan"
> > +#define LED_FUNCTION_LINK "link"
> >  #define LED_FUNCTION_MAIL "mail"
> >  #define LED_FUNCTION_MTD "mtd"
> >  #define LED_FUNCTION_PANIC "panic"  
> 
> We have function "lan" already defined; "link" would do mostly same
> thing. Should we use "lan"? Or should we delete "lan" and replace it
> with "link"?

Removal of "lan" should depend on whether it is used somewhere...

But I think "link" is more sensible since:
- it can distinguish better from "activity" if "activity" should mean
  blinking on RX/TX
- the interface must not necessarily be a LAN (in the sense that on
  routers we have WAN and LAN and possibly other, user defined
  networks).

We could use "lan" though to mean "link" and "activity" together (ie.
LED on when interface linked, LED blinking on RX/TX).

We have to decide whether specifying more LED functions to one LED in
DT should look like:

  function = LED_FUNCTION_LINK, LED_FUNCTION_ACTIVITY;

or rather like

  function = LED_FUNCTION_LAN;

The problem with the second one is that we would need specific
functions for different compound modes (if we wanted to do that), eg:
  function = LED_FUNCTION_LAN_RX;

Marek