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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ