[PATCH v2 1/2] regulator: dummy: force synchronous probing

[PATCH v2 1/2] regulator: dummy: force synchronous probing

[Christian Eggers]

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,
 	},
 };
 
-- 
2.44.1

[PATCH v2 2/2] regulator: check that dummy regulator has been probed before using it

[Christian Eggers]

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;
 
-- 
2.44.1

Re: [PATCH v2 2/2] regulator: check that dummy regulator has been probed before using it

[Doug Anderson]

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.

Re: [PATCH v2 2/2] regulator: check that dummy regulator has been probed before using it

[Christian Eggers]

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

Re: [PATCH v2 1/2] regulator: dummy: force synchronous probing

[Mark Brown]

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

Re: [PATCH v2 1/2] regulator: dummy: force synchronous probing

[Doug Anderson]

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

Re: [PATCH v2 1/2] regulator: dummy: force synchronous probing

[Christian Eggers]

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