Message-ID: <Z_4_8wnetwpoWuwG@smile.fi.intel.com>
Date: Tue, 15 Apr 2025 14:16:03 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Ivan Vecera <ivecera@...hat.com>
Cc: Andrew Lunn <andrew@...n.ch>, 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>,
	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 Tue, Apr 15, 2025 at 12:01:43PM +0200, Ivan Vecera wrote:
> 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?

Sounds like a good plan to me, but I'm not in the exact area of this driver's
interest, so others may have better suggestions.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists