Sometimes I get a NULL pointer dereference at boot time in kobject_get() with the following call stack:
anatop_regulator_probe() devm_regulator_register() regulator_register() regulator_resolve_supply() kobject_get()
By placing some extra BUG_ON() statements I could verify that this is raised because probing of the 'dummy' regulator driver is not completed ('dummy_regulator_rdev' is still NULL).
In the JTAG debugger I can see that dummy_regulator_probe() and anatop_regulator_probe() can be run by different kernel threads (kworker/u4:*). I haven't further investigated whether this can be changed or if there are other possibilities to force synchronization between these two probe routines. On the other hand I don't expect much boot time penalty by probing the 'dummy' regulator synchronously.
Cc: stable@vger.kernel.org Fixes: 259b93b21a9f ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in 4.14") Signed-off-by: Christian Eggers ceggers@arri.de --- v2: - no changes
drivers/regulator/dummy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/regulator/dummy.c b/drivers/regulator/dummy.c index 5b9b9e4e762d..9f59889129ab 100644 --- a/drivers/regulator/dummy.c +++ b/drivers/regulator/dummy.c @@ -60,7 +60,7 @@ static struct platform_driver dummy_regulator_driver = { .probe = dummy_regulator_probe, .driver = { .name = "reg-dummy", - .probe_type = PROBE_PREFER_ASYNCHRONOUS, + .probe_type = PROBE_FORCE_SYNCHRONOUS, }, };
Due to asynchronous driver probing there is a chance that the dummy regulator hasn't already been probed when first accessing it.
Cc: stable@vger.kernel.org Signed-off-by: Christian Eggers ceggers@arri.de --- v2: - return -EPROBE_DEFER rather than using BUG_ON()
drivers/regulator/core.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 4ddf0efead68..4276493ce7c6 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -2069,6 +2069,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
if (have_full_constraints()) { r = dummy_regulator_rdev; + if (!r) { + ret = -EPROBE_DEFER; + goto out; + } get_device(&r->dev); } else { dev_err(dev, "Failed to resolve %s-supply for %s\n", @@ -2086,6 +2090,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) goto out; } r = dummy_regulator_rdev; + if (!r) { + ret = -EPROBE_DEFER; + goto out; + } get_device(&r->dev); }
@@ -2213,6 +2221,8 @@ struct regulator *_regulator_get_common(struct regulator_dev *rdev, struct devic */ dev_warn(dev, "supply %s not found, using dummy regulator\n", id); rdev = dummy_regulator_rdev; + if (!rdev) + return ERR_PTR(-EPROBE_DEFER); get_device(&rdev->dev); break;
Hi,
On Tue, Mar 11, 2025 at 2:18 AM Christian Eggers ceggers@arri.de wrote:
@@ -2213,6 +2221,8 @@ struct regulator *_regulator_get_common(struct regulator_dev *rdev, struct devic */ dev_warn(dev, "supply %s not found, using dummy regulator\n", id); rdev = dummy_regulator_rdev;
if (!rdev)return ERR_PTR(-EPROBE_DEFER);
nit: it feels like the dev_warn() above should be below your new check. Otherwise you'll get the same message again after the deferral processes.
On Thursday, 13 March 2025, 01:46:43 CET, Doug Anderson wrote:
Hi,
On Tue, Mar 11, 2025 at 2:18 AM Christian Eggers ceggers@arri.de wrote:
@@ -2213,6 +2221,8 @@ struct regulator *_regulator_get_common(struct regulator_dev *rdev, struct devic */ dev_warn(dev, "supply %s not found, using dummy regulator\n", id); rdev = dummy_regulator_rdev;
if (!rdev)return ERR_PTR(-EPROBE_DEFER);nit: it feels like the dev_warn() above should be below your new check. Otherwise you'll get the same message again after the deferral processes.
I actually had a similar feeling during making the change. Having entropy on warning messages isn't very nice, so I'll send a v3.
regards Christian
On Tue, 11 Mar 2025 10:18:02 +0100, Christian Eggers wrote:
Sometimes I get a NULL pointer dereference at boot time in kobject_get() with the following call stack:
anatop_regulator_probe() devm_regulator_register() regulator_register() regulator_resolve_supply() kobject_get()
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/2] regulator: dummy: force synchronous probing commit: 8619909b38eeebd3e60910158d7d68441fc954e9 [2/2] regulator: check that dummy regulator has been probed before using it commit: b60ef2a3334ca15f249188870fc029ddf06ef7c4
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
Hi,
On Tue, Mar 11, 2025 at 2:18 AM Christian Eggers ceggers@arri.de wrote:
Sometimes I get a NULL pointer dereference at boot time in kobject_get() with the following call stack:
anatop_regulator_probe() devm_regulator_register() regulator_register() regulator_resolve_supply() kobject_get()
By placing some extra BUG_ON() statements I could verify that this is raised because probing of the 'dummy' regulator driver is not completed ('dummy_regulator_rdev' is still NULL).
In the JTAG debugger I can see that dummy_regulator_probe() and anatop_regulator_probe() can be run by different kernel threads (kworker/u4:*). I haven't further investigated whether this can be changed or if there are other possibilities to force synchronization between these two probe routines. On the other hand I don't expect much boot time penalty by probing the 'dummy' regulator synchronously.
Cc: stable@vger.kernel.org Fixes: 259b93b21a9f ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in 4.14") Signed-off-by: Christian Eggers ceggers@arri.de
v2:
- no changes
drivers/regulator/dummy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Not that it should really hurt, but do we need both commit cfaf53cb472e ("regulator: check that dummy regulator has been probed before using it") and this one? It seems like commit cfaf53cb472e ("regulator: check that dummy regulator has been probed before using it") would be sufficient and we don't really need to force the regulator to synchronous probing.
...not that I expect the dummy probing synchronously to be a big deal, I just want to make sure I understand.
-Doug
Hi Doug,
On Thursday, 13 March 2025, 01:42:14 CET, Doug Anderson wrote:
Hi,
On Tue, Mar 11, 2025 at 2:18 AM Christian Eggers ceggers@arri.de wrote:
Sometimes I get a NULL pointer dereference at boot time in kobject_get() with the following call stack:
anatop_regulator_probe() devm_regulator_register() regulator_register() regulator_resolve_supply() kobject_get()
By placing some extra BUG_ON() statements I could verify that this is raised because probing of the 'dummy' regulator driver is not completed ('dummy_regulator_rdev' is still NULL).
In the JTAG debugger I can see that dummy_regulator_probe() and anatop_regulator_probe() can be run by different kernel threads (kworker/u4:*). I haven't further investigated whether this can be changed or if there are other possibilities to force synchronization between these two probe routines. On the other hand I don't expect much boot time penalty by probing the 'dummy' regulator synchronously.
Cc: stable@vger.kernel.org Fixes: 259b93b21a9f ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in 4.14") Signed-off-by: Christian Eggers ceggers@arri.de
v2:
- no changes
drivers/regulator/dummy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Not that it should really hurt, but do we need both commit cfaf53cb472e ("regulator: check that dummy regulator has been probed before using it") and this one? It seems like commit cfaf53cb472e ("regulator: check that dummy regulator has been probed before using it") would be sufficient and we don't really need to force the regulator to synchronous probing.
actually I also tested successfully without synchronous probing (only with checking that the dummy regulator has been probed) and this also worked fine (just to be sure, I also added a temporary delay in the dummy's probe routine). But as the dummy regulator doesn't rely on slow I/O, I felt that synchronous probing makes more sense than "busy-waiting" for it.
...not that I expect the dummy probing synchronously to be a big deal, I just want to make sure I understand.
-Doug
regards, Christian
linux-stable-mirror@lists.linaro.org
-
Christian Eggers -
Doug Anderson -
Mark Brown