linux-kernel - Re: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access

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, 22 Feb 2022 00:19:14 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Alvin Šipraga <alvin@...s.dk>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Michael Rasmussen <MIR@...g-olufsen.dk>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Arınç ÜNAL <arinc.unal@...nc9.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize
 indirect PHY register access

Hi Linus,

Linus Walleij <linus.walleij@...aro.org> writes:

> On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <alvin@...s.dk> wrote:
>
>> Realtek switches in the rtl8365mb family can access the PHY registers of
>> the internal PHYs via the switch registers. This method is called
>> indirect access. At a high level, the indirect PHY register access
>> method involves reading and writing some special switch registers in a
>> particular sequence. This works for both SMI and MDIO connected
>> switches.
>
> What I worry about is whether we need to do a similar patch to
> rtl8366rb_phy_read() and rtl8366rb_phy_write() in
> rtl8366rb.c?

Unfortunately I do not have the hardware to test rtl8366rb.c, so I can
only speculate. But I gave some hints in the commit message which might
help in checking whether or not it is an issue on that hardware as
well. The code for rtl8366rb_phy_read() looks similar, but since this is
a quirk of the hardware design, it could be that it is not
necessary. The only way is to test.

If you or somebody else can confirm that it is an issue for RTL8366RB as
well, I will happily send a patch to the list for testing. You can for
example try spamming PHY register reads with phytool while also reading
off switch registers via regmap debugfs. See also the discussion in [1].

[1] https://lore.kernel.org/netdev/878rukib4f.fsf@bang-olufsen.dk/

>
> And what about the upcoming rtl8367 driver?

Is this a hypothetical driver or have I missed something on the list? If
you mean the changes Luiz has sent for net-next, then it is covered by
this series. Those switches (RTL8367S, RTL8367RB?) are in the same
family as the RTL8365MB-VC and are supported by rtl8365mb.c.

>
>> To fix this problem, one must guard against regmap access while the
>> PHY indirect register read is executing. Fix this by using the newly
>> introduced "nolock" regmap in all PHY-related functions, and by aquiring
>> the regmap mutex at the top level of the PHY register access callbacks.
>> Although no issue has been observed with PHY register _writes_, this
>> change also serializes the indirect access method there. This is done
>> purely as a matter of convenience and for reasons of symmetry.
>>
>> Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC")
>> Link: https://lore.kernel.org/netdev/CAJq09z5FCgG-+jVT7uxh1a-0CiiFsoKoHYsAWJtiKwv7LXKofQ@mail.gmail.com/
>> Link: https://lore.kernel.org/netdev/871qzwjmtv.fsf@bang-olufsen.dk/
>> Reported-by: Arınç ÜNAL <arinc.unal@...nc9.com>
>> Reported-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
>> Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
>
> This is a beautiful patch. Excellent job.
> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

Thank you Linus for your kind words.

Kind regards,
Alvin