Hi Linus,
Linus Walleij <[email protected]> writes:
> On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <[email protected]> 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/[email protected]/
>
> 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/[email protected]/
>> Reported-by: Arınç ÜNAL <[email protected]>
>> Reported-by: Luiz Angelo Daros de Luca <[email protected]>
>> Signed-off-by: Alvin Šipraga <[email protected]>
>
> This is a beautiful patch. Excellent job.
> Reviewed-by: Linus Walleij <[email protected]>
Thank you Linus for your kind words.
Kind regards,
Alvin
On Tue, Feb 22, 2022 at 1:19 AM Alvin Šipraga <[email protected]> wrote:
> > On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <[email protected]> wrote:
> > 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.
I can test it.
> 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/[email protected]/
If you have time to write a patch I'd love testing it!
Yours,
Linus Walleij
Linus Walleij <[email protected]> writes:
> On Tue, Feb 22, 2022 at 1:19 AM Alvin Šipraga <[email protected]> wrote:
>> > On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <[email protected]> wrote:
>
>> > 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.
>
> I can test it.
Great! I have attached a test patch to insert these stray register
accesses inside the PHY read/write functions:
0001-TEST-net-dsa-realtek-rtl8366rb-provoke-stray-reg-acc.patch
Set the module parameter stray_access to 1 in order to enable stray
register access. I did this in case you don't build it as a module but
still want to do A/B testing - just remove it if I goofed.
>
>> 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/[email protected]/
>
> If you have time to write a patch I'd love testing it!
Attached also a patch with a proposed fix - if indeed it is an issue -
along the same lines as the second patch in this series. You'll also
need the first patch from this series (attached for convenience):
0001-net-dsa-realtek-allow-subdrivers-to-externally-lock-.patch
0002-TEST-net-dsa-realtek-rtl8366rb-serialize-indirect-PH.patch
All patches are against net-next and build-tested only, so you might
want to double check if the results are unexpected.
Kind regards,
Alvin
>
> Yours,
> Linus Walleij