| Message ID | 20241211-syscon-fixes-v1-3-b5ac8c219e96@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mfd: syscon: Cleanup, fix race condition and remove platform driver | expand |
Hi Rob, Thanks for working on this! On 12/11/2024, Rob Herring (Arm) wrote: > of_syscon_register_regmap() was added for nodes which need a custom > regmap setup. It's not really correct for those nodes to claim they are > compatible with "syscon" as the default handling likely doesn't work in > those cases. If device_node_get_regmap() happens to be called first, > then of_syscon_register() will be called and an incorrect regmap will be > created (barring some other error). That may lead to unknown results in > the worst case. In the best case, of_syscon_register_regmap() will fail > with -EEXIST. This problem remains unless these cases drop "syscon" (an > ABI issue) or we exclude them using their specific compatible. ATM, > there is only one user: "google,gs101-pmu" > > There are also cases of adding "syscon" compatible to existing nodes > after the fact in order to register the syscon. That presents a > potential DT ABI problem. Instead, if there's a kernel change needing a > syscon for a node, then it should be possible to allow the kernel to > register a syscon without a DT change. That's only possible by using > of_syscon_register_regmap() currently, but in the future we may want to > support a match list for cases which don't need a custom regmap. > > With this change, the lookup functions will succeed for any node > registered by of_syscon_register_regmap() regardless of whether the node > compatible contains "syscon". > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org> > --- > drivers/mfd/syscon.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index bfb1f69fcff1..e6df2825c14d 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -171,8 +171,10 @@ static struct regmap *device_node_get_regmap(struct device_node *np, > break; > } > > - if (!syscon) > + if (!syscon && of_device_is_compatible(np, "syscon")) > syscon = of_syscon_register(np, check_res); > + else > + syscon = ERR_PTR(-EINVAL); This else case actually breaks Pixel 6 (Tensor) since you are now returning -EINVAL when the syscon is created by the exynos-pmu driver and present in the list. Instead you should only return -EINVAL if the syscon doesn't exist and the device node is not a compatible syscon device. If you still want to check for `of_device_is_compatible(np, "syscon")` even when the syscon is found in the list, that should be okay , but it's probably best to check that before inserting the regmap in the list to begin with. This worked for me on my Pixel 6 device: if (!syscon) { if (of_device_is_compatible(np, "syscon")) syscon = of_syscon_register(np, check_res); else syscon = ERR_PTR(-EINVAL); } Thanks, Will
Hi Rob, On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote: > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index bfb1f69fcff1..e6df2825c14d 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -171,8 +171,10 @@ static struct regmap *device_node_get_regmap(struct device_node *np, > break; > } > > - if (!syscon) > + if (!syscon && of_device_is_compatible(np, "syscon")) > syscon = of_syscon_register(np, check_res); > + else > + syscon = ERR_PTR(-EINVAL); The current modification will make device_node_get_regmap() return -EINVAL even for syscons that were already found in the syscon_list, which I believe is not the intended behavior. I suggest modifying it this way to maintain lookup functionality for registered syscons while implementing your intended changes: static struct regmap *device_node_get_regmap(struct device_node *np, bool check_res) { struct syscon *entry, *syscon = NULL; struct regmap *regmap; mutex_lock(&syscon_list_lock); list_for_each_entry(entry, &syscon_list, list) if (entry->np == np) { syscon = entry; break; } if (syscon) { regmap = syscon->regmap; mutex_unlock(&syscon_list_lock); return regmap; } if (of_device_is_compatible(np, "syscon")) { syscon = of_syscon_register(np, check_res); mutex_unlock(&syscon_list_lock); if (IS_ERR(syscon)) return ERR_CAST(syscon); return syscon->regmap; } mutex_unlock(&syscon_list_lock); return ERR_PTR(-EINVAL); } The above is the resulting working function I've obtained while testing with different scenarios. This ensures that: 1. Already registered syscons are found and returned correctly 2. New syscons with "syscon" compatible are registered as before 3. Nodes without "syscon" compatible return -EINVAL only if not found in the list This has been tested with my v1 syscon work [1] and with v2, where I remove the "syscon" compatible string for custom regmap initialization, aligning with your goals for this series. Let me know if I should add this series as a dependency of my v2 or how I should proceed. Thanks, John Madieu [1] https://lore.kernel.org/all/20241206212559.192705-2-john.madieu.xa@bp.renesas.com
On Sun, Dec 15, 2024 at 2:34 PM John Madieu <john.madieu.xa@bp.renesas.com> wrote: > > Hi Rob, > > On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote: > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > > index bfb1f69fcff1..e6df2825c14d 100644 > > --- a/drivers/mfd/syscon.c > > +++ b/drivers/mfd/syscon.c > > @@ -171,8 +171,10 @@ static struct regmap *device_node_get_regmap(struct device_node *np, > > break; > > } > > > > - if (!syscon) > > + if (!syscon && of_device_is_compatible(np, "syscon")) > > syscon = of_syscon_register(np, check_res); > > + else > > + syscon = ERR_PTR(-EINVAL); > > The current modification will make device_node_get_regmap() return -EINVAL even > for syscons that were already found in the syscon_list, which I believe is not > the intended behavior. Yes, it is. Doesn't Will's fix work for you? > > I suggest modifying it this way to maintain lookup functionality for registered > syscons while implementing your intended changes: > > static struct regmap *device_node_get_regmap(struct device_node *np, > bool check_res) > { > struct syscon *entry, *syscon = NULL; > struct regmap *regmap; > > mutex_lock(&syscon_list_lock); > > list_for_each_entry(entry, &syscon_list, list) > if (entry->np == np) { > syscon = entry; > break; > } > > if (syscon) { > regmap = syscon->regmap; > mut ix_unlock(&syscon_list_lock); > return regmap; > } > > if (of_device_is_compatible(np, "syscon")) { > syscon = of_syscon_register(np, check_res); > mutex_unlock(&syscon_list_lock); > if (IS_ERR(syscon)) > return ERR_CAST(syscon); > return syscon->regmap; > } > > mutex_unlock(&syscon_list_lock); 3 unlock calls is a sign the code structure could be improved. A goto or a guard() for example. However, I think this is the same logic as what Will suggested. Rob
Hi Rob, > -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: Monday, December 16, 2024 6:40 PM > To: John Madieu <john.madieu.xa@bp.renesas.com> > Cc: arnd@arndb.de; heiko@sntech.de; krzysztof.kozlowski@linaro.org; > lee@kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; liviu.dudau@arm.com; lpieralisi@kernel.org; > pankaj.dubey@samsung.com; peter.griffin@linaro.org; sudeep.holla@arm.com; > willmcvicker@google.com; Biju Das <biju.das.jz@bp.renesas.com> > Subject: Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a > "syscon" compatible > > On Sun, Dec 15, 2024 at 2:34 PM John Madieu > <john.madieu.xa@bp.renesas.com> wrote: > > > > Hi Rob, > > > > On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote: > > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index > > > bfb1f69fcff1..e6df2825c14d 100644 > > > --- a/drivers/mfd/syscon.c > > > +++ b/drivers/mfd/syscon.c > > > @@ -171,8 +171,10 @@ static struct regmap > *device_node_get_regmap(struct device_node *np, > > > break; > > > } > > > > > > - if (!syscon) > > > + if (!syscon && of_device_is_compatible(np, "syscon")) > > > syscon = of_syscon_register(np, check_res); > > > + else > > > + syscon = ERR_PTR(-EINVAL); > > > > The current modification will make device_node_get_regmap() return > > -EINVAL even for syscons that were already found in the syscon_list, > > which I believe is not the intended behavior. > > Yes, it is. Doesn't Will's fix work for you? Did not see Will's answer while doing my tests. I however tested it and it works. I did also test your v2 series, which worked with me as well. I'll then drop syscon compatible string in my series and send the v2. Thanks, John > > > > > I suggest modifying it this way to maintain lookup functionality for > > registered syscons while implementing your intended changes: > > > > static struct regmap *device_node_get_regmap(struct device_node *np, > > bool check_res) { > > struct syscon *entry, *syscon = NULL; > > struct regmap *regmap; > > > > mutex_lock(&syscon_list_lock); > > > > list_for_each_entry(entry, &syscon_list, list) > > if (entry->np == np) { > > syscon = entry; > > break; > > } > > > > if (syscon) { > > regmap = syscon->regmap; > > mut ix_unlock(&syscon_list_lock); > > return regmap; > > } > > > > if (of_device_is_compatible(np, "syscon")) { > > syscon = of_syscon_register(np, check_res); > > mutex_unlock(&syscon_list_lock); > > if (IS_ERR(syscon)) > > return ERR_CAST(syscon); > > return syscon->regmap; > > } > > > > mutex_unlock(&syscon_list_lock); > > 3 unlock calls is a sign the code structure could be improved. A goto or a > guard() for example. However, I think this is the same logic as what Will > suggested. > > Rob
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index bfb1f69fcff1..e6df2825c14d 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -171,8 +171,10 @@ static struct regmap *device_node_get_regmap(struct device_node *np, break; } - if (!syscon) + if (!syscon && of_device_is_compatible(np, "syscon")) syscon = of_syscon_register(np, check_res); + else + syscon = ERR_PTR(-EINVAL); mutex_unlock(&syscon_list_lock); @@ -238,9 +240,6 @@ EXPORT_SYMBOL_GPL(device_node_to_regmap); struct regmap *syscon_node_to_regmap(struct device_node *np) { - if (!of_device_is_compatible(np, "syscon")) - return ERR_PTR(-EINVAL); - return device_node_get_regmap(np, true); } EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
of_syscon_register_regmap() was added for nodes which need a custom regmap setup. It's not really correct for those nodes to claim they are compatible with "syscon" as the default handling likely doesn't work in those cases. If device_node_get_regmap() happens to be called first, then of_syscon_register() will be called and an incorrect regmap will be created (barring some other error). That may lead to unknown results in the worst case. In the best case, of_syscon_register_regmap() will fail with -EEXIST. This problem remains unless these cases drop "syscon" (an ABI issue) or we exclude them using their specific compatible. ATM, there is only one user: "google,gs101-pmu" There are also cases of adding "syscon" compatible to existing nodes after the fact in order to register the syscon. That presents a potential DT ABI problem. Instead, if there's a kernel change needing a syscon for a node, then it should be possible to allow the kernel to register a syscon without a DT change. That's only possible by using of_syscon_register_regmap() currently, but in the future we may want to support a match list for cases which don't need a custom regmap. With this change, the lookup functions will succeed for any node registered by of_syscon_register_regmap() regardless of whether the node compatible contains "syscon". Signed-off-by: Rob Herring (Arm) <robh@kernel.org> --- drivers/mfd/syscon.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)