Message-ID: <9de10e97-d0fa-4dee-b98a-e4b2a3f7019c@redhat.com>
Date: Tue, 15 Apr 2025 12:01:43 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Andrew Lunn <andrew@...n.ch>, Andy Shevchenko <andy@...nel.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, netdev@...r.kernel.org,
 Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
 Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
 Jiri Pirko <jiri@...nulli.us>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Prathosh Satish <Prathosh.Satish@...rochip.com>,
 Lee Jones <lee@...nel.org>, Kees Cook <kees@...nel.org>,
 Andy Shevchenko <andy@...nel.org>, Andrew Morton
 <akpm@...ux-foundation.org>, Michal Schmidt <mschmidt@...hat.com>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register
 defs

On 10. 04. 25 11:54 odp., Andrew Lunn wrote:
> ...
> 
> So a small number of registers in the regmap need special locking. It
> was not clear to me what exactly those locking requirements are,
> because they don't appear to be described.
> 
> But when i look at the code above, the scoped guard gives the
> impression that i have to read id, revision, fw_vr and cfg_ver all in
> one go without any other reads/writes happening. I strongly suspect
> that impression is wrong. The question then becomes, how can i tell
> apart reads/writes which do need to be made as one group, form others
> which can be arbitrarily ordered with other read/writes.
> 
> What i suggest you do is try to work out how to push the locking down
> as low as possible. Make the lock cover only what it needs to cover.
> 
> Probably for 95% of the registers, the regmap lock is sufficient.
> 
> Just throwing out ideas, i've no idea if they are good or not. Create
> two regmaps onto your i2c device, covering different register
> ranges. The 'normal' one uses standard regmap locking, the second
> 'special' one has locking disabled. You additionally provide your own
> lock functions to the 'normal' one, so you have access to the
> lock. When you need to access the mailboxes, take the lock, so you
> know the 'normal' regmap cannot access anything, and then use the
> 'special' regmap to do what you need to do. A structure like this
> should help explain what the special steps are for those special
> registers, while not scattering wrong ideas about what the locking
> scheme actually is all over the code.

Hi Andrew,
the idea looks interesting but there are some caveats and disadvantages.
I thought about it but the idea with two regmaps (one for simple 
registers and one for mailboxes) where the simple one uses implicit 
locking and mailbox one has locking disabled with explicit locking 
requirement. There are two main problems:

1) Regmap cache has to be disabled as it cannot be shared between 
multiple regmaps... so also page selector cannot be cached.

2) You cannot mix access to mailbox registers and to simple registers. 
This means that mailbox accesses have to be wrapped e.g. inside 
scoped_guard()

The first problem is really pain as I would like to extend later the 
driver with proper caching (page selector for now).
The second one brings only confusions for a developer how to properly 
access different types of registers.

I think the best approach would be to use just single regmap for all 
registers with implicit locking enabled and have extra mailbox mutex to 
protect mailbox registers and ensure atomic operations with them.
This will allow to use regmap cache and also intermixing mailbox and 
simple registers' accesses won't be an issue.

@Andy Shevchenko, wdym about it?

Thanks,
Ivan

Powered by blists - more mailing lists