Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220216160500.2341255-1-alvin@pqrs.dk> <CAJq09z6Mr7QFSyqWuM1jjm9Dis4Pa2A4yi=NJv1w4FM0WoyqtA@mail.gmail.com>
 <87k0dusmar.fsf@bang-olufsen.dk>
In-Reply-To: <87k0dusmar.fsf@bang-olufsen.dk>
From:   Luiz Angelo Daros de Luca <luizluca@gmail.com>
Date:   Thu, 17 Feb 2022 01:28:48 -0300
Message-ID: <CAJq09z70QyuyNtQVBW+jWOZ-CgY3uvyTo95JkMvCFNvOs2S1dw@mail.gmail.com>
Subject: Re: [PATCH net-next 0/2] net: dsa: realtek: fix PHY register read corruption
To:     =?UTF-8?Q?Alvin_=C5=A0ipraga?= <ALSI@bang-olufsen.dk>
Cc:     =?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>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Michael Rasmussen <MIR@bang-olufsen.dk>,
        =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        "open list:NETWORKING DRIVERS" <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > I still feel like we are trying to go around a regmap limitation
> > instead of fixing it there. If we control regmap lock (we can define a
> > custom lock/unlock) and create new regmap_{read,write}_nolock
> > variants, we'll just need to lock the regmap, do whatever you need,
> > and unlock it.
>
> Can you show me what those regmap_{read,write}_nolock variants would
> look like in your example? And what about the other regmap_ APIs we use,
> like regmap_read_poll_timeout, regmap_update_bits, etc. - do you propose
> to reimplement all of these?

The option of having two regmaps is a nice way to have "_nolock"
variants for free. It is much cleaner than any solutions I imagined!
Ayway, I don't believe the regmap API expects to have an evil
non-locked clone. It looks like it is being abused.

What regmap API misses is a way to create a "transaction". Mdio, for
example, expects the user to lock the bus before doing a series of
accesses while regmap api assumes a single atomic access is enough.
However, Realtek indirect register access shows that it is not enough.
We could reimplement a mutex for every case where two calls might
share the same register (or indirectly affect others like we saw with
Realtek) but I believe a shared solution would be better, even if it
costs a couple more wrap functions.

It would be even nicer if we have a regmap "manual lock" mode that
will expose the lock/unlock functions but it will never call them by
itself. It would work if it could check if the caller is actually the
same thread/context that locked it. However I doubt there is a clean
solution in a kernel code that can check if the lock was acquired by
the same context that is calling the read.


> > BTW, I believe that, for realtek-mdio, a regmap custom lock mechanism
> > could simply use mdio lock while realtek-smi already has priv->lock.
>
> Hmm OK. Actually I'm a bit confused about the mdio_lock: can you explain
> what it's guarding against, for someone unfamiliar with MDIO? Currently
> realtek-mdio's regmap has an additional lock around it (disable_locking
> is 0), so with these patches applied the number of locks remains the
> same.

Today we already have to redundants locks (mdio and regmap). Your
patch is just replacing the regmap lock.

regmap_read is something like this:

regmap_read
    lock regmap
    realtek_mdio_read()
        lock mdio
        ...
        unlock mdio
   unlock regmap

If you are implementing a custom lock, simply use mdio lock directly.

And the map_nolock you created does not mean "access without locks"
but "you must lock it yourself before using anything here". If that
lock is actually mdio_lock, it would be ok to remove the lock inside
realtek_mdio_{read,write}. You just need a reference to those
lock/unlock functions in realtek_priv.

> priv->lock is a spinlock which is inappropriate here. I'm not really
> sure what the point of it is, besides to handle unlocked calls to the
> _noack function. It might be removable altogether but I would prefer not
> to touch it for this series.

If spinlock is inappropriate, it can be easily converted to a mutex.
Everything else from realtek-mdio might apply.

> Kind regards,
> Alvin