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:   Thu, 10 Feb 2022 23:48:59 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
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

Luiz Angelo Daros de Luca <luizluca@...il.com> writes:

>> This assumes a 1:1 mapping between the port number and its PHY address
>> on the internal MDIO bus, is that always true?
>
> Thanks Florian,
>
> 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.

>
>> It seems to me like we are resisting as much as possible the creating of
>> the MDIO bus using of_mdiobus_register() and that seems to be forcing
>> you to jump through hoops to get your per-port PHY interrupts mapped.
>>
>> Maybe this needs to be re-considered and you should just create that
>> internal MDIO bus without the help of the DSA framework and reap the
>> benefits? We could also change the DSA framework's way of creating the
>> MDIO bus so as to be OF-aware.
>
> That looks like a nice idea.
>
> I do not have any problem duplicating the mdio setup from realtek-smi
> into realtek-mdio.
> However, it is just 3 copies of the same code (and I believe there are
> a couple more of them):
>
> 1) dsa_switch_setup()+dsa_slave_mii_bus_init()
> 2) realtek_smi_setup_mdio()
> 3) realtek_mdio_setup_mdio() (NEW)
>
> And realtek_smi_setup_mdio only exists as a way to reference the
> OF-node. And OF-node is only needed because it needs to associate the
> interrupt-parent and interrupts with each phy.
> I think the best solution would be a way that the
> dsa_slave_mii_bus_init could look for a specific subnode. Something
> like:
>
> dsa_slave_mii_bus_init(struct dsa_switch *ds)
> {
>         struct device_node *dn;
> ...
>         dn = of_get_child_by_name(ds->dn, "slave_mii_bus");
>         if (dn) {
>                 ds->slave_mii_bus->dev.of_node = dn;
>         }
> ...
> }
>
> It would remove the realtek_smi_setup_mdio().

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"). 

>
> 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?

Kind regards,
Alvin

>
> Does it sound ok?
>
> --
> Luiz