Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220216160500.2341255-1-alvin@pqrs.dk> <20220216160500.2341255-3-alvin@pqrs.dk>
 <20220216233906.5dh67olhgfz7ji6o@skbuf> <CAJq09z6XBQUTBZoQ81Vy3nUc_5QGTF0GH8V-S3bXOw=JpYODvA@mail.gmail.com>
 <87v8xdrjpw.fsf@bang-olufsen.dk>
In-Reply-To: <87v8xdrjpw.fsf@bang-olufsen.dk>
From:   Luiz Angelo Daros de Luca <luizluca@gmail.com>
Date:   Mon, 21 Feb 2022 21:18:17 -0300
Message-ID: <CAJq09z4sbh1zWKd-yiQpeV1H_1fEU6f7uhsH69JmTXcb4YEVZg@mail.gmail.com>
Subject: Re: [PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: serialize
 indirect PHY register access
To:     =?UTF-8?Q?Alvin_=C5=A0ipraga?= <ALSI@bang-olufsen.dk>
Cc:     Vladimir Oltean <olteanv@gmail.com>,
        =?UTF-8?Q?Alvin_=C5=A0ipraga?= <alvin@pqrs.dk>,
        Linus Walleij <linus.walleij@linaro.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        Michael Rasmussen <MIR@bang-olufsen.dk>,
        "open list:NETWORKING DRIVERS" <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > The realtek "API/driver" does exactly how the driver was doing. They
> > do have a lock/unlock placeholder, but only in the equivalent
> > regmap_{read,write} functions. Indirect access does not use locks at
> > all (in fact, there is no other mention of "lock" elsewhere), even
> > being obvious that it is not thread-safe. It was just with a DSA
> > driver that we started to exercise register access for real, specially
> > without interruptions. And even in that case, we could only notice
> > this issue in multicore devices. I believe that, if they know about
> > this issue, they might not be worried because it has never affected a
> > real device. It would be very interesting to hear from Realtek but I
> > do not have the contacts.
>
> This is not true, at least with the sources I am reading. As I said in
> my reply to Vladimir, the Realtek code takes a lock around each
> top-level API call. Example:
>
> rtk_api_ret_t rtk_port_phyStatus_get(...)
> {
>     rtk_api_ret_t retVal;
>
>     if (NULL == RT_MAPPER->port_phyStatus_get)
>         return RT_ERR_DRIVER_NOT_FOUND;
>
>     RTK_API_LOCK();
>     retVal = RT_MAPPER->port_phyStatus_get(port, pLinkStatus, pSpeed, pDuplex);
>     RTK_API_UNLOCK();
>
>     return retVal;
> }
>
> Deep down in this port_phyStatus_get() callback, the indirect PHY
> register access takes place.

For the record, in the rtl8367c driver I'm using as reference, there
is no mention of RTK_API_LOCK(). Check here same function you copied:

https://github.com/openwrt/openwrt/blob/aae7af4219e56c2787f675109d9dd1a44a5dcba4/target/linux/mediatek/files-5.10/drivers/net/phy/rtk/rtl8367c/port.c#L1003-L1040

So, this indirect reg access protection is something they added along
the way, probably when they started to use it with SMP systems.

> Kind regards,
> Alvin