netdev - Re: [PATCH RFC net-next 0/8] DSA LED infrastructure, mv88e6xxx and QCA8K

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]

Date: Wed, 29 Nov 2023 17:27:00 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev <netdev@...r.kernel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Christian Marangi <ansuelsmth@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH RFC net-next 0/8] DSA LED infrastructure, mv88e6xxx and
 QCA8K

On Wed, Nov 29, 2023 at 05:43:36PM +0200, Vladimir Oltean wrote:
> On Wed, Nov 29, 2023 at 04:13:00PM +0100, Andrew Lunn wrote:
> > O.K, i need to think about this.
> > 
> > What is not obvious to me at the moment is how we glue the bits
> > together. I don't want each DSA driver having to parse the DSA part of
> > the DT representation. So the DSA core needs to call into this library
> > while parsing the DT to create the LEDs. We also need an ops structure
> > in the DSA driver which this library can use. We then need to
> > associate the ops structure the driver has with the LEDs the DSA core
> > creates in the library. Maybe we can use ds->dev as a cookie.
> > 
> > Before i get too deep into code, i will post the basic API idea for a
> > quick review.
> 
> What is the DSA portion of the DT representation? I see "leds" goes
> under the generic ethernet-controller.yaml.

I agree the properties are well defined. The problem is finding them.

       switch@0 {
                compatible = "marvell,mv88e6085";
                #address-cells = <1>;
                #size-cells = <0>;
                reg = <0>;

                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        port@0 {
                                reg = <0>;
                                label = "lan4";

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

                                        led@0 {
                                                reg = <0>;
                                                color = <LED_COLOR_ID_WHITE>;
                                                function = LED_FUNCTION_LAN;
                                                label = "front";
                                                default-state = "keep";
                                        };
                                };
                        };

                        port@1 {
                                reg = <1>;
                                label = "lan3";

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

                                        led@0 {
                                                reg = <0>;
                                                color = <LED_COLOR_ID_WHITE>;
                                                function = LED_FUNCTION_LAN;
                                                label = "front";
                                                default-state = "keep";
                                        };
                                };
                        };


I don't want each DSA driver having to walk this tree to find the leds
node to pass it to a library to create the LEDs. We already have code
do to this walk in the DSA core. So one option would be the DSA core
does the call to the library as it performs the walk.

Now that i've looked at the code, the core does set dp->dn to point to
the port node. So setup_port() could do the call into the library to
create the LEDs, and pass it the ops structure. That seems clean, and
should avoid DSA core changes you don't like.

       Andrew