[PATCH] mfd: syscon: Set max_register_is_0 when syscon points to a single register

[PATCH] mfd: syscon: Set max_register_is_0 when syscon points to a single register

[Nishanth Menon]

Commit 0ec74ad3c157 ("regmap: rework ->max_register handling")
introduced explicit handling in regmap framework for register maps
that is exactly 1 register wide. As a result, a syscon pointing
to a single register would cause regmap checks to skip checks
(or in the case of regmap_get_max_register, return -EINVAL) as
max_register_is_set will not be true.

Fixes: 0ec74ad3c157 ("regmap: rework ->max_register handling")
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Cc: Jan Dakinevich <jan.dakinevich@salutedevices.com>
Cc: Mark Brown <broonie@kernel.org>

WARNING:
This is probably going to expose some other hidden bugs in other code
bases, for example: https://lore.kernel.org/r/20240827131342.6wrielete3yeoinl@bryanbrattlof.com
exposed a very interesting behavior - regmap_read now correctly fails
with -EIO in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/ti-cpufreq.c#n343
when the dt is modified to provide exact syscon register, but cpufreq
driver attempts to read at 0x18 reg offset, but then, the code assumes
that invalid regmap ranges imply OMAP3 and goes and reads some random
register.. (fixes for cpufreq is pending).

in the above example: adding prints in cpufreq driver (in the same
location pointed above):
before this patch:
read returns 0 with value 0x00000243 and regmap_get_max_register would
return -EINVAL

After this patch:
read returns -EIO with value 0x00000000 and regmap_get_max_register would
return 0x0

This would be the correct behavior.

 drivers/mfd/syscon.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 2ce15f60eb10..3e1d699ba934 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -108,6 +108,8 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 	syscon_config.reg_stride = reg_io_width;
 	syscon_config.val_bits = reg_io_width * 8;
 	syscon_config.max_register = resource_size(&res) - reg_io_width;
+	if (!syscon_config.max_register)
+		syscon_config.max_register_is_0 = true;
 
 	regmap = regmap_init_mmio(NULL, base, &syscon_config);
 	kfree(syscon_config.name);
@@ -357,6 +359,9 @@ static int syscon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	syscon_config.max_register = resource_size(res) - 4;
+	if (!syscon_config.max_register)
+		syscon_config.max_register_is_0 = true;
+
 	if (pdata)
 		syscon_config.name = pdata->label;
 	syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config);

-- 
2.43.0

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Wed, Aug 28, 2024 at 07:10:08AM -0500, Nishanth Menon wrote:

> Commit 0ec74ad3c157 ("regmap: rework ->max_register handling")
> introduced explicit handling in regmap framework for register maps
> that is exactly 1 register wide. As a result, a syscon pointing
> to a single register would cause regmap checks to skip checks
> (or in the case of regmap_get_max_register, return -EINVAL) as
> max_register_is_set will not be true.

In what sense is the behaviour changed for a map that doesn't specify a
maximum register?

> Fixes: 0ec74ad3c157 ("regmap: rework ->max_register handling")

In what sense is this a fix?

> +	if (!syscon_config.max_register)
> +		syscon_config.max_register_is_0 = true;

This will cause any syscon which does not explicitly specify a maximum
register to be converted to having only one register at number 0.  That
really does not seem like a good idea - unless you've done an audit of
every single syscon to make sure they do explicitly specify a maximum
register, and confirmed that this can't be specified via DT, then it's
going to break things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Nishanth Menon]

On 13:57-20240828, Mark Brown wrote:
> On Wed, Aug 28, 2024 at 07:10:08AM -0500, Nishanth Menon wrote:
> 
> > Commit 0ec74ad3c157 ("regmap: rework ->max_register handling")
> > introduced explicit handling in regmap framework for register maps
> > that is exactly 1 register wide. As a result, a syscon pointing
> > to a single register would cause regmap checks to skip checks
> > (or in the case of regmap_get_max_register, return -EINVAL) as
> > max_register_is_set will not be true.
> 
> In what sense is the behaviour changed for a map that doesn't specify a
> maximum register?
> 
> > 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.

> 
> > +	if (!syscon_config.max_register)
> > +		syscon_config.max_register_is_0 = true;
> 
> This will cause any syscon which does not explicitly specify a maximum
> register to be converted to having only one register at number 0.  That

The context of the diff is important - code above already sets
the max_register as syscon_config.max_register = resource_size(&res) -
reg_io_width;

So it already does set the max_register. syscon does'nt explictly set a
max_reg - it is derived from the resource_size.

> really does not seem like a good idea - unless you've done an audit of
> every single syscon to make sure they do explicitly specify a maximum
> register, and confirmed that this can't be specified via DT, then it's
> going to break things.

I understand the risk - but having a consistent max_register definition
is important - key here is that in regmap, max_register is valid if:
a) max_register not being 0
b) if max_register is 0, it is valid only if max_register_is_0 is set to
true.

When syscon sets the max_register, it operates correctly for num_reg > 1
however, when reg_size == 1, you don't get the checks that you
get when num_regs > 1. That is inconsistent behavior.

It might help if you can clarify why you think an inconsistent behavior
is correct for syscon?

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

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.

> > really does not seem like a good idea - unless you've done an audit of
> > every single syscon to make sure they do explicitly specify a maximum
> > register, and confirmed that this can't be specified via DT, then it's
> > going to break things.

> I understand the risk - but having a consistent max_register definition
> is important - key here is that in regmap, max_register is valid if:
> a) max_register not being 0
> b) if max_register is 0, it is valid only if max_register_is_0 is set to
> true.

> When syscon sets the max_register, it operates correctly for num_reg > 1
> however, when reg_size == 1, you don't get the checks that you
> get when num_regs > 1. That is inconsistent behavior.

> It might help if you can clarify why you think an inconsistent behavior
> is correct for syscon?

Like I say specifying a maximum register is entirely optional, not
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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Nishanth Menon]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

On Wed, Aug 28, 2024 at 09:44:34AM -0500, Nishanth Menon wrote:

> 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.

If you are absolutely confident that all syscon users know how big their
regmap is then modulo the claim that it's a fix for an unrelated patch
which doesn't change the behaviour for these regmaps at all then it's
fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Nishanth Menon]

On 21:26-20240828, Mark Brown wrote:
> On Wed, Aug 28, 2024 at 09:44:34AM -0500, Nishanth Menon wrote:
> 
> > 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.
> 
> If you are absolutely confident that all syscon users know how big their
> regmap is then modulo the claim that it's a fix for an unrelated patch
> which doesn't change the behaviour for these regmaps at all then it's
> fine.

Thanks for clarifying - I understand now.. I will drop the fixes tag
and refresh the patch with appropriate wording - but, to the best of
my search[1] ("Absolutely confident" is a pretty strong terminology for
any patch ;)) there is expectation of max_registers being enforced..
based on what the driver does today.

[1] https://gist.github.com/nmenon/d537096d041fa553565fba7577d2cd24 ->
 the pattern seems relatively controlled problem that existing code
 as far as I can see doesn't seem to mis-behave.

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