linux-kernel - Re: [PATCH] mfd: syscon: Set max_register_is_0 when syscon points to a single register

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]

Message-ID: <20240828144434.oydsgflsqy5vibxe@sapling>
Date: Wed, 28 Aug 2024 09:44:34 -0500
From: Nishanth Menon <nm@...com>
To: Mark Brown <broonie@...nel.org>
CC: Arnd Bergmann <arnd@...db.de>, Lee Jones <lee@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <afd@...com>, <bb@...com>, <d-gole@...com>,
        Jan Dakinevich
	<jan.dakinevich@...utedevices.com>
Subject: Re: [PATCH] mfd: syscon: Set max_register_is_0 when syscon points to
 a single register

Mark,

On 15:27-20240828, Mark Brown wrote:
> On Wed, Aug 28, 2024 at 08:32:29AM -0500, Nishanth Menon wrote:
> > On 13:57-20240828, Mark Brown wrote:
> > > On Wed, Aug 28, 2024 at 07:10:08AM -0500, Nishanth Menon wrote:
> 
> > > > Fixes: 0ec74ad3c157 ("regmap: rework ->max_register handling")
> 
> > > In what sense is this a fix?
> 
> > The max_register was 0x0 was clearly a corner case. The fix done for
> > remap  should have cleaned up the users of max_register to maintain the
> > behavior. That is just my opinion.
> 
> No, apart from the fact that catching all possible regmap users would be
> rather hard it's entirely optional for regmaps to specify a maxium
> register.

Fair enough, I can drop the 'Fixes' tag.

[...]

> Like I say specifying a maximum register is entirely optional, not

I understand max_register is entirely optional as documented in
include/linux/regmap.h and I understand why max_register_is_0 exists -
the patch does NOT try to revert the valid feature in regmap.

> everyone wants that feature and if you don't use the debugfs interface
> or the flat cache it doesn't *super* matter.  With 0 as default it's
> always going to be awkward to describe a maximum register of 0 while
> allowing that to be optional, fortunately very few devices have a single
> register.

yeah, unfortunately I have to deal with a few of them :(

This is a patch for syscon, not regmap. I am a bit confused as to what
objection beyond the "Fixes" usage (which I can drop
in a respin) you may have here, will appreciate if you are NAKing the
patch and on what rationale.

I understand that regmap considers the max_register usage entirely
optional, but syscon does already use it (my patch doesn't introduce
it). I am just getting syscon to catchup to what regmap already
provides.

Side effect of leaving the current syscon code as is creating bad
results that should can be trivially caught (and could have saved me
a couple of hours of early morning head scratching today[1]). I think
the regmap checks are valuable, and should be consistent in usage by
syscon. And I think the checks are valuable for programmers from
mistakenly introducing bad code.

[1] Already pointed out in diffstat https://lore.kernel.org/all/20240828131601.6sxvnwpcsb36tz4m@eloquent/

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D