netdev - Re: [PATCH net-next v2 2/5] dt-bindings: net: dsa: ksz: add mdio-parent-bus property for internal MDIO

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: <20241029123107.ssvggsn2b5w3ehoy@skbuf>
Date: Tue, 29 Oct 2024 14:31:07 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	Arun Ramadoss <arun.ramadoss@...rochip.com>,
	Conor Dooley <conor+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Rob Herring <robh+dt@...nel.org>, Rob Herring <robh@...nel.org>,
	kernel@...gutronix.de, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
	"Russell King (Oracle)" <linux@...linux.org.uk>,
	devicetree@...r.kernel.org, Marek Vasut <marex@...x.de>
Subject: Re: [PATCH net-next v2 2/5] dt-bindings: net: dsa: ksz: add
 mdio-parent-bus property for internal MDIO

On Tue, Oct 29, 2024 at 12:07:29PM +0100, Oleksij Rempel wrote:
> Introduce `mdio-parent-bus` property in the ksz DSA bindings to
> reference the parent MDIO bus when the internal MDIO bus is attached to
> it, bypassing the main management interface.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> Reviewed-by: Rob Herring (Arm) <robh@...nel.org>
> Reviewed-by: Andrew Lunn <andrew@...n.ch>
> ---
>  .../devicetree/bindings/net/dsa/microchip,ksz.yaml       | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> index a4e463819d4d7..121a4bbd147be 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -84,6 +84,15 @@ properties:
>    mdio:
>      $ref: /schemas/net/mdio.yaml#
>      unevaluatedProperties: false
> +    properties:
> +      mdio-parent-bus:
> +        $ref: /schemas/types.yaml#/definitions/phandle
> +        description:
> +          Phandle pointing to the MDIO bus controller connected to the
> +          secondary MDIO interface. This property should be used when
> +          the internal MDIO bus is accessed via a secondary MDIO
> +          interface rather than the primary management interface.
> +
>      patternProperties:
>        "^ethernet-phy@[0-9a-f]$":
>          type: object
> -- 
> 2.39.5
> 

I'm not saying whether this is good or bad, I'm just worried about
mixing quantities having different measurement units into the same
address space.

Just like in the case of an mdio-mux, there is no address space isolation
between the parent bus and the child bus. AKA you can't have this,
because there would be clashes:

	host_bus: mdio@...d {
		ethernet-phy@2 {
			reg = <2>;
		};
	};

	child_bus: mdio@...h {
		mdio-parent-bus = <&host_bus>;

		ethernet-phy@2 {
			reg = <2>;
		};
	};

But there is a big difference. With an mdio-mux, you could statically
detect address space clashes by inspecting the PHY addresses on the 2
buses. But with the lan937x child MDIO bus, in this design, you can't,
because the "reg" values don't represent MDIO addresses, but switch port
numbers (this is kind of important, but I don't see it mentioned in the
dt-binding). These are translated by lan937x_create_phy_addr_map() using
the CASCADE_ID/VPHY_ADD pin strapping information read over SPI.
I.e. with the same device tree, you may or may not have address space
clashes depending on pin strapping. No way to tell.

Have you considered putting the switch's internal PHYs directly under
the host MDIO bus node, with their 'real' MDIO bus computed statically
by the DT writer based on the pin straps? Yes, I'm aware that this means
different pin straps mean different device trees.

Under certain circumstances I could understand this dt-binding design
with an mdio-parent-bus, like for example if the MDIO addresses at which
the internal PHYs respond would be configurable over SPI, from the switch
registers. But I'm not led to believe that here, this is the case.