netdev - Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-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]

Date:   Fri, 11 Feb 2022 01:51:16 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Arınç ÜNAL <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio

Thanks Alvin,

> > As far as I know, for supported models, yes. I'm not sure about models
> > rtl8363nb and rtl8364nb because they have only 2 user ports at 1 and
> > 3.
> > Anyway, they are not supported yet.
>
> I think the port number as defined in the device tree is always going to
> be the same as its PHY address on the internal bus. I had a look at the
> Realtek code and this seems to be the assumption there too.

One of the realtek-smi.txt examples (that I also copied to
realtek.yaml) does not respect that:

phy4: phy@4 {
   reg = <4>;
   interrupt-parent = <&switch_intc>;
   interrupts = <12>;
};

I don't know if 12 is a typo here.

It would only matter if I do create a default association when the
specific device tree-entry is missing. It wasn't supposed to
completely remove the device-tree declaration but to make it optional.
For now, I'll put this option aside.

> >> We could also change the DSA framework's way of creating the
> >> MDIO bus so as to be OF-aware.

It worked like a charm. I'll send it in reply to this email. I still
have some questions about it.

> We are not the only ones doing this. mv88e6xxx is another example. So
> Florian's suggestion seems like a good one, but we should be careful to
> maintain compatibility with older device trees. In some cases it is
> based on child node name (e.g. "mdio"), in others it is based on the
> child node compatible string (e.g. "realtek,smi-mdio",
> "marvell,mv88e6xxx-mdio-external").

The name "mdio" seems to be the de facto name. I'll use it. However,
it might be confusing with mdio-connected switches as you'll have an
mdio inside a switch inside another mdio. But it is exactly what it
is.

It would not affect drivers that are already allocating slave_mii_bus
by themselves. If the driver is fine with that, that's the end of the
case.

However, if a driver doesn't need any special properties inside the
mdio node, it might want to migrate to the default dsa slave_mii_bus.
For those already using "mdio" node name, they just need to drop their
code and move phy_read/write to dsa_switch_ops. Now, those using
different node names (like when they check the compatible strings)
will have a little more job. I believe we cannot rename a node "on the
fly". So, if the matched node name is not mdio, they still need to
allocate the slave_mii_bus. They will also need a different
dsa_switch_ops for each case because dsa_switch_ops->phy_read cannot
coexist with an externally allocated slave_mii_bus. Each driver needs
to plan their own migration path if they want to migrate.

For realtek-smi, the code does not require the node to be named "mdio"
but doc "realtek-smi.txt" and my new "realtek.yaml" does require it.
If I assume that, I could simply drop the code and migrate to
read/write to dsa_switch_ops.
If not, we need to maintain both code paths and warn the user for a
couple of releases until we drop the compatible string match.

> > If possible, I would like to define safe default values (like assuming
> > 1:1 mapping between the port number and its PHY address) for this
> > driver when interrupt-controller is present but
> > slave_mii_bus node is missing.
>
> You could just require the phy nodes to be described in the device
> tree. Then you don't need this extra port_setup code. Seems better IMO,
> or am I missing something?

Upstream devs seem to prefer more code than more device-tree confs. I
just wanted to reduce some device-tree copy/paste. I'm ok with using a
device-tree node.

---
Luiz