netdev - Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <c609a7f865ab48f858adafdd9c1014dda8ec82d6.camel@svanheule.net>
Date:   Sun, 29 Jan 2023 21:43:00 +0100
From:   Sander Vanheule <sander@...nheule.net>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, Rob Herring <robh@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>, Pavel Machek <pavel@....cz>,
        "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
        John Crispin <john@...ozen.org>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-leds@...r.kernel.org,
        Tim Harvey <tharvey@...eworks.com>,
        Alexander Stein <alexander.stein@...tq-group.com>,
        Rasmus Villemoes <rasmus.villemoes@...vas.dk>
Subject: Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs
 definition example

Hi Christian,

On Wed, 2022-12-21 at 13:54 +0100, Christian Marangi wrote:
> For reg it's really specific to the driver... My idea was that since a
> single phy can have multiple leds attached, reg will represent the led
> number.
> 
> This is an example of the dt implemented on a real device.
> 
>                 mdio {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         phy_port1: phy@0 {
>                                 reg = <0>;
> 
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
[...]
>                                 };
>                         };
[...]
>                 };
> 
> In the following implementation. Each port have 2 leds attached (out of
> 3) one white and one amber. The driver parse the reg and calculate the
> offset to set the correct option with the regs by also checking the phy
> number.

With switch silicon allowing user control of the LEDs, vendors can (and will)
use the switch's LED peripheral to drive other LEDs (or worse). E.g. on a Cisco
SG220-26 switch, using a Realtek RTL8382 SoC, the LEDs associated with some
unused switch ports are used to display a global device status. My concern here
is that one would have to specify switch ports, that aren't connected to
anything, just to describe those non-ethernet LEDs.

Would an alternative with a 'trigger-sources' property pointing to the right phy
be an option? The trade-off I see would be that extra port info has to be
provided on a separate LED controller, which your example can avoid thanks to
the phy's reg property.

Building on your example this may become:

       switch {
           mdio {
                #address-cells = <1>;
                #size-cells = <0>;
                
                switch_phy0: phy@0 {
                    reg = <0>;
                    #trigger-source-cells = <1>;
                };
            };

            leds {
                #address-cells = <2>;
                #size-cells = <0>;

                /* First port, first LED */
                /* Port status, can be offloaded */
                led@0.0 {
                    reg = <0 0>;
                    trigger-sources = <&switch_phy0 (NET_LINK | NET_SPEED_1000)>;
                    function = color = <LED_COLOR_ID_WHITE>;
                    function = LED_FUNCTION_LAN;
                    function-enumerator = <1>;
                    linux,default-trigger = "netdev";
                };

                /* First port, first LED */
                /* Port status, can be offloaded */
                led@0.1 {
                    reg = <0 1>;
                    trigger-sources = <&switch_phy0 (NET_LINK | NET_SPEED_100 | NET_SPEED_10)>;
                    function = color = <LED_COLOR_ID_AMBER>;
                    function = LED_FUNCTION_LAN;
                    function-enumerator = <1>;
                    linux,default-trigger = "netdev";
                };

                /* Last port (not used in hardware), first LED */
                /* Device status, software controlled */
                led@7.0 {
                    reg = <7 0>;
                    function = color = <LED_COLOR_ID_AMBER>;
                    function = LED_FUNCTION_STATUS;
                    linux,default-trigger = "default-on";
                };
            };
        };


To be a bit less verbose, the &switch_mdio node might serve as trigger provider
with a single cell, but the above would allow only defined phy-s to be
referenced.

The trigger-source cells could be used for a more fine grained control of what
should be offloaded (link up/down, Rx/Tx activity, link speed, ...). Although
this selectivity is most likely runtime configurable, this could serve as a
description of static device labeling (e.g. "LINK/ACT 1000").

Switching to the implementation and driver side, the 'trigger-sources' property
could be used by the netdev trigger to determine if a status LED can be
offloaded. The netdev trigger could just hide the whole hardware/software
control aspect then. Much like how the timer trigger always offloads if an
implementation is provided, even when offloading is less flexible than the
software implementation of the timer trigger.


Best,
Sander