netdev - Re: [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap

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:   Mon, 21 Feb 2022 21:06:19 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Alvin Šipraga <alvin@...s.dk>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Michael Rasmussen <mir@...g-olufsen.dk>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Arınç ÜNAL <arinc.unal@...nc9.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to
 externally lock regmap

On Mon, Feb 21, 2022 at 07:46:30PM +0100, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@...g-olufsen.dk>
> 
> Currently there is no way for Realtek DSA subdrivers to serialize
> consecutive regmap accesses. In preparation for a bugfix relating to
> indirect PHY register access - which involves a series of regmap
> reads and writes - add a facility for subdrivers to serialize their
> regmap access.
> 
> Specifically, a mutex is added to the driver private data structure and
> the standard regmap is initialized with custom lock/unlock ops which use
> this mutex. Then, a "nolock" variant of the regmap is added, which is
> functionally equivalent to the existing regmap except that regmap
> locking is disabled. Functions that wish to serialize a sequence of
> regmap accesses may then lock the newly introduced driver-owned mutex
> before using the nolock regmap.
> 
> Doing things this way means that subdriver code that doesn't care about
> serialized register access - i.e. the vast majority of code - needn't
> worry about synchronizing register access with an external lock: it can
> just continue to use the original regmap.
> 
> Another advantage of this design is that, while regmaps with locking
> disabled do not expose a debugfs interface for obvious reasons, there
> still exists the original regmap which does expose this interface. This
> interface remains safe to use even combined with driver codepaths that
> use the nolock regmap, because said codepaths will use the same mutex
> to synchronize access.
> 
> With respect to disadvantages, it can be argued that having
> near-duplicate regmaps is confusing. However, the naming is rather
> explicit, and examples will abound.
> 
> Finally, while we are at it, rename realtek_smi_mdio_regmap_config to
> realtek_smi_regmap_config. This makes it consistent with the naming
> realtek_mdio_regmap_config in realtek-mdio.c.
> 
> Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@...il.com>

>  drivers/net/dsa/realtek/realtek-mdio.c | 46 ++++++++++++++++++++++--
>  drivers/net/dsa/realtek/realtek-smi.c  | 48 ++++++++++++++++++++++++--
>  drivers/net/dsa/realtek/realtek.h      |  2 ++
>  3 files changed, 91 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 0308be95d00a..31e1f100e48e 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -98,6 +98,20 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
>  	return ret;
>  }
>  
> +static void realtek_mdio_lock(void *ctx)

I looked even at the build results for v1 and I don't know exactly why
sparse doesn't complain about the lack of __acquires() and __releases()
annotations, to avoid context imbalance warnings.

https://patchwork.kernel.org/project/netdevbpf/patch/20220216160500.2341255-2-alvin@pqrs.dk/

Anyway, those aren't of much use IMO (you can't get it to do anything
but very trivial checks), and if sparse doesn't complain for whatever
reason, I certainly won't request you to add them.

I see other implementations don't have them either - like vexpress_config_lock.