netdev - Re: [PATCH net-next] net: stmmac: don't create the MDIO bus if there's no mdio node on DT

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: Tue, 8 Aug 2023 20:51:01 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andrew Lunn <andrew@...n.ch>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, 
	"David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alex Elder <elder@...aro.org>, 
	Srini Kandagatla <srinivas.kandagatla@...aro.org>, Andrew Halaney <ahalaney@...hat.com>, 
	netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH net-next] net: stmmac: don't create the MDIO bus if
 there's no mdio node on DT

On Tue, Aug 8, 2023 at 8:46 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Tue, Aug 08, 2023 at 02:02:54PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >
> > The stmmac_dt_phy() function that parses the device-tree node of the MAC
> > and allocates the MDIO and PHY resources misses one use-case: when the
> > MAC doesn't have a fixed link but also doesn't define its own mdio bus
> > on the device tree and instead shares the MDIO lines with a different
> > MAC with its PHY phandle reaching over into a different node.
>
> It does not share the MDIO lines. The other MDIO bus master happens to
> have two PHYs and there are no PHYs on this MDIO bus, so no point
> instantiating it.

Yes, I sent it before we established that thanks to Andrew's input.

>
> >  static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> >                        struct device_node *np, struct device *dev)
> >  {
> > -     bool mdio = !of_phy_is_fixed_link(np);
> >       static const struct of_device_id need_mdio_ids[] = {
> >               { .compatible = "snps,dwc-qos-ethernet-4.10" },
> >               {},
> >       };
> >
> > +     if (of_phy_is_fixed_link(np))
> > +             return 0;
> > +
>
>                 /**
>                  * If snps,dwmac-mdio is passed from DT, always register
>                  * the MDIO
>                  */
>                 for_each_child_of_node(np, plat->mdio_node) {
>                         if (of_device_is_compatible(plat->mdio_node,
>                                                     "snps,dwmac-mdio"))
>                                 break;
>                 }
>
> The comment suggests it should always be registered. This MAC might
> have a fixed-phy, but that does not mean there is not an Ethernet
> switch on the bus, or a PHY for some other MAC etc. MDIO busses
> masters should be considered fully independent devices.
>
> https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts
>
> &fec1 {
>         phy-mode = "rmii";
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_fec1>;
>         status = "okay";
>
>         fixed-link {
>                 speed = <100>;
>                 full-duplex;
>         };
>
>         mdio1: mdio {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 clock-frequency = <12500000>;
>                 suppress-preamble;
>                 status = "okay";
>
>                 switch0: switch0@0 {
>                         compatible = "marvell,mv88e6190";
>                         pinctrl-0 = <&pinctrl_gpio_switch0>;
>                         pinctrl-names = "default";
>                         reg = <0>;
>                         eeprom-length = <65536>;
>                         interrupt-parent = <&gpio3>;
>                         interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>
> Both a fixed link, and something on the MDIO bus....
>
>      Andrew

Makes sense, we can drop all my stmmac patches from today, I need to
rethink it in detail.

Bart