| Message ID | 20240828121008.3066002-1-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mfd: syscon: Set max_register_is_0 when syscon points to a single register | expand |
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.
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?
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.
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/
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.
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.
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);
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(+)