Date:   Thu, 17 Feb 2022 01:28:48 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     Alvin Šipraga <alvin@...s.dk>,
        Linus Walleij <linus.walleij@...aro.org>,
        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>,
        Arınç ÜNAL <arinc.unal@...nc9.com>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 0/2] net: dsa: realtek: fix PHY register read corruption

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

Powered by blists - more mailing lists