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:   Sun, 9 Oct 2022 01:56:28 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jerry Ray <jerry.ray@...rochip.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next][PATCH v4] dt-bindings: dsa: Add lan9303 yaml

On Mon, Oct 03, 2022 at 11:46:24AM -0500, Jerry Ray wrote:
> ---
> v3->v4:
>  - Addressed v3 community feedback

More specifically?

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    // Ethernet switch connected via mdio to the host
> +    ethernet {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        phy-handle = <&lan9303switch>;
> +        phy-mode = "rmii";
> +        fixed-link {
> +            speed = <100>;
> +            full-duplex;
> +        };

I see the phy-handle to the switch is inherited from the .txt dt-binding,
but I don't understand it. The switch is an mdio_device, not a phy_device,
so what will this do?

Also, any reasonable host driver will error out if it finds a phy-handle
and a fixed-link in its OF node. So one of phy-handle or fixed-link must
be dropped, they are bogus.

Even better, just stick to the mdio node as root and drop the DSA master
OF node, like other DSA dt-binding examples do. You can have dangling
phandles, so "ethernet = <&ethernet>" below is not an issue.

> +        mdio {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            lan9303switch: switch@0 {
> +                compatible = "smsc,lan9303-mdio";
> +                reg = <0>;
> +                dsa,member = <0 0>;

Redundant, please remove.

> +                ethernet-ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                        port@0 {
> +                            reg = <0>;
> +                            phy-mode = "rmii";

FWIW, RMII has a MAC mode and a PHY mode. Two RMII interfaces connected
in MAC mode to one another don't work. You'll have problems if you also
have an RMII PHY connected to one of the xMII ports, and you describe
phy-mode = "rmii" for both. There exists a "rev-rmii" phy-mode to denote
an RMII interface working in PHY mode. Wonder if you should be using
that here.

> +                            ethernet = <&ethernet>;
> +                            fixed-link {
> +                                speed = <100>;
> +                                full-duplex;
> +                            };
> +                        };
> +                        port@1 {
> +                            reg = <1>;
> +                            max-speed = <100>;
> +                            label = "lan1";
> +                        };
> +                        port@2 {
> +                            reg = <2>;
> +                            max-speed = <100>;
> +                            label = "lan2";
> +                        };
> +                    };
> +                };
> +            };
> +        };
> +
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    // Ethernet switch connected via i2c to the host
> +    ethernet {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        phy-mode = "rmii";
> +            speed = <100>;
> +        fixed-link {
> +            full-duplex;
> +        };
> +    };

No need for this node.

> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        lan9303: switch@1a {
> +            compatible = "smsc,lan9303-i2c";
> +            reg = <0x1a>;
> +            ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                port@0 {
> +                    reg = <0>;
> +                    phy-mode = "rmii";
> +                    ethernet = <&ethernet>;
> +                    fixed-link {
> +                        speed = <100>;
> +                        full-duplex;
> +                    };
> +                };
> +                port@1 {
> +                    reg = <1>;
> +                    max-speed = <100>;
> +                    label = "lan1";
> +                };
> +                port@2 {
> +                    reg = <2>;
> +                    max-speed = <100>;
> +                    label = "lan2";
> +                };
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5d58b55c5ae5..89055ff2838a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13386,6 +13386,14 @@ L:	netdev@...r.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/microchip/lan743x_*
>  
> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER
> +M:	Jerry Ray <jerry.ray@...rochip.com>
> +M:	UNGLinuxDriver@...rochip.com
> +L:	netdev@...r.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml
> +F:	drivers/net/dsa/lan9303*
> +

Separate patch please? Changes to the MAINTAINERS file get applied to
the "net" tree.

>  MICROCHIP LAN966X ETHERNET DRIVER
>  M:	Horatiu Vultur <horatiu.vultur@...rochip.com>
>  M:	UNGLinuxDriver@...rochip.com
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists