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

[Vladimir Oltean]

On Mon, Feb 21, 2022 at 07:46:30PM +0100, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
> ---

Reviewed-by: Vladimir Oltean <olteanv@xxxxxxxxx>

>  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@xxxxxxx/

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.