[PATCH] regulator: core: Unset supplies when regulators are unregistered

[PATCH] regulator: core: Unset supplies when regulators are unregistered

[Jon Hunter]

When regulators are unregistered they are removed from the regulator map
list, however, if the register has been added as a supply for another
regulator, it is not removed from the regulator as a supply. Therefore,
a regulator may still be holding a reference to a regulator that has
been unregistered a lead to a panic.

There is a case where a child regulator is registered before its supply
and when the supply is registered successfully, the supply for the child
is then set. Although, this in itself is not a problem, a problem arises
when the supply is then unregistered again, due to the parent device
being probed deferred, after successfully registering the supply. This
leaves the regulator with an invalid reference to supply and can
eventually result in a kernel panic.

Addtionally, even in the normal case when a regulator is unregistered,
by unloading a driver, if the regulator happens to be a supply for
another regulator it is not removed.

Fix this by scanning all the registered regulators when a regulator is
removed and remove it as a supply to any other regulator. There is a
possibility that a child regulator is in use when the supply is removed
and so WARN if this happens.

Note that the debugfs node for the regulator supply must be freed before
the debugfs node for the regulator_dev and so ensure the regulator_dev
debugfs node is freed after the any supplies have been removed.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---

This problem has been exposed on -next due to some other changes that
have caused a slight re-ordering in the boot sequence for Tegra124
Jetson-TK1 and causes the boot to fail. The board panics when trying
to enable a supply which is not valid. This happens because the supply
for a regulator is resolved when the supply is registered but then the
supply gets unregistered again due to a probe deferral later on,
leaving a invalid reference to the supply for the regulator.

I can't say I am completely happy with this as there is still the
potential for someone to remove a supply while a child regulator is
in use. However, I guess that is no different from today AFAICT, but
hopefully this will avoid some panics?!?

Ideally, it would be good to defer all the supply resolution until we
know for certain that the device has been probed OK and will not be
deferred in anyway. So I am not sure if there is a better way to fix
this.

 drivers/regulator/core.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 04baac9..18b22fd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1242,6 +1242,21 @@ static int set_consumer_device_supply(struct regulator_dev *rdev,
 	return 0;
 }
 
+static int regulator_unset_supply(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+	struct regulator_dev *supply_rdev = data;
+
+	if (rdev->supply && rdev->supply->rdev == supply_rdev) {
+		rdev_dbg(rdev, "removing supply %s\n", rdev_get_name(rdev));
+		WARN_ON(rdev->open_count);
+		_regulator_put(rdev->supply);
+		rdev->supply = NULL;
+	}
+
+	return 0;
+}
+
 static void unset_regulator_supplies(struct regulator_dev *rdev)
 {
 	struct regulator_map *node, *n;
@@ -1253,6 +1268,9 @@ static void unset_regulator_supplies(struct regulator_dev *rdev)
 			kfree(node);
 		}
 	}
+
+	class_for_each_device(&regulator_class, NULL, rdev,
+			      regulator_unset_supply);
 }
 
 #ifdef CONFIG_DEBUG_FS
@@ -4131,10 +4149,10 @@ void regulator_unregister(struct regulator_dev *rdev)
 		regulator_put(rdev->supply);
 	}
 	mutex_lock(&regulator_list_mutex);
-	debugfs_remove_recursive(rdev->debugfs);
 	flush_work(&rdev->disable_work.work);
-	WARN_ON(rdev->open_count);
 	unset_regulator_supplies(rdev);
+	debugfs_remove_recursive(rdev->debugfs);
+	WARN_ON(rdev->open_count);
 	list_del(&rdev->list);
 	regulator_ena_gpio_free(rdev);
 	mutex_unlock(&regulator_list_mutex);
-- 
1.9.1

Re: [PATCH] regulator: core: Unset supplies when regulators are unregistered

[Mark Brown]

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

On Fri, Jan 06, 2017 at 11:54:38AM +0000, Jon Hunter wrote:

> There is a case where a child regulator is registered before its supply
> and when the supply is registered successfully, the supply for the child
> is then set. Although, this in itself is not a problem, a problem arises
> when the supply is then unregistered again, due to the parent device
> being probed deferred, after successfully registering the supply. This
> leaves the regulator with an invalid reference to supply and can
> eventually result in a kernel panic.

Why is a parent device doing this?  This doesn't seem like safe or
helpful behaviour and with probe deferral we'd generally expect the
device to acquire resources before it starts making use of them.

> Addtionally, even in the normal case when a regulator is unregistered,
> by unloading a driver, if the regulator happens to be a supply for
> another regulator it is not removed.

We can't completely stop people doing this but we do make fairly strong
efforts to stop people pulling in use devices.

> Fix this by scanning all the registered regulators when a regulator is
> removed and remove it as a supply to any other regulator. There is a
> possibility that a child regulator is in use when the supply is removed
> and so WARN if this happens.

This seems like storing up trouble for the future, we'll end up with
live child devices with parents that weren't around or being refcounted
through some of the lifetime of the device which will doubtless come
back and bite us later.

> Note that the debugfs node for the regulator supply must be freed before
> the debugfs node for the regulator_dev and so ensure the regulator_dev
> debugfs node is freed after the any supplies have been removed.

Please don't mix different changes into one commit, as covered in
SubmittingPatches please send one patch per change.  This makes things
easier to 

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

Re: [PATCH] regulator: core: Unset supplies when regulators are unregistered

[Jon Hunter]

On 06/01/17 18:29, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Jan 06, 2017 at 11:54:38AM +0000, Jon Hunter wrote:
> 
>> There is a case where a child regulator is registered before its supply
>> and when the supply is registered successfully, the supply for the child
>> is then set. Although, this in itself is not a problem, a problem arises
>> when the supply is then unregistered again, due to the parent device
>> being probed deferred, after successfully registering the supply. This
>> leaves the regulator with an invalid reference to supply and can
>> eventually result in a kernel panic.
> 
> Why is a parent device doing this?  This doesn't seem like safe or
> helpful behaviour and with probe deferral we'd generally expect the
> device to acquire resources before it starts making use of them.

It is not really the parent that is doing this. The child regulator in
this case is physically a separate regulator from the main PMIC device
that is registering the supply regulators.

The PMIC is registering N regulators and after registering some
successfully one fails because it's supply is not found and the
regulator core returns -EPROBE_DEFER. Normally not finding supply would
not fail but in this case it is necessary because the regulator is in
bypass and the supply is needed to get the voltage.

Actually, this particular regulator was the cause of some other issues
in the past where we needed to fix-up the handling of regulators in
bypass during registration. See commit 45389c47526d ('regulator: core:
Add early supply resolution for regulators').

The real problem is that after one of the PMIC's regulators fails to
register, by then one of the PMIC's other regulators (that has already
been registered) has been added as a supply for another external regulator.

>> Addtionally, even in the normal case when a regulator is unregistered,
>> by unloading a driver, if the regulator happens to be a supply for
>> another regulator it is not removed.
> 
> We can't completely stop people doing this but we do make fairly strong
> efforts to stop people pulling in use devices.

Right, but before removing/unregistering a regulator today, we never
check to see if it is the supply for any other regulator.

>> Fix this by scanning all the registered regulators when a regulator is
>> removed and remove it as a supply to any other regulator. There is a
>> possibility that a child regulator is in use when the supply is removed
>> and so WARN if this happens.
> 
> This seems like storing up trouble for the future, we'll end up with
> live child devices with parents that weren't around or being refcounted
> through some of the lifetime of the device which will doubtless come
> back and bite us later.

Yes I am not completely happy. Maybe we should wait for the parent
device to be bound before allowing any of its's regulators to be added
as supplies for other regulators? I gave the following a quick test and
this seems to work too ...

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 04baac9a165b..e17915861a54 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1553,6 +1553,11 @@ static int regulator_resolve_supply(struct
regulator_dev *rdev)
                }
        }

+       if (!device_is_bound(r->dev.parent)) {
+               put_device(&r->dev);
+               return -EPROBE_DEFER;
+       }
+
        /* Recursively resolve the supply of the supply */
        ret = regulator_resolve_supply(r);
        if (ret < 0) {


The above prevents anyone from using a regulator as a supply if the
parent has not been bound yet. Therefore, if one of the parent's
regulators fails to register, after some have already been successfully
registered, there is no chance any of the successfully added regulators
will have been already added as a supply to some other regulator in the
system. Hope that's clear!

>> Note that the debugfs node for the regulator supply must be freed before
>> the debugfs node for the regulator_dev and so ensure the regulator_dev
>> debugfs node is freed after the any supplies have been removed.
> 
> Please don't mix different changes into one commit, as covered in
> SubmittingPatches please send one patch per change.  This makes things
> easier to 

Sorry, may be I worded this badly, however, it is a consequence of this
particular change and otherwise it would not be needed.

Cheers
Jon

-- 
nvpublic

Re: [PATCH] regulator: core: Unset supplies when regulators are unregistered

[Mark Brown]

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

On Mon, Jan 09, 2017 at 01:37:18PM +0000, Jon Hunter wrote:
> On 06/01/17 18:29, Mark Brown wrote:

> > Why is a parent device doing this?  This doesn't seem like safe or
> > helpful behaviour and with probe deferral we'd generally expect the
> > device to acquire resources before it starts making use of them.

> It is not really the parent that is doing this. The child regulator in
> this case is physically a separate regulator from the main PMIC device
> that is registering the supply regulators.

The parent of the regulator being unregistered.

> > We can't completely stop people doing this but we do make fairly strong
> > efforts to stop people pulling in use devices.

> Right, but before removing/unregistering a regulator today, we never
> check to see if it is the supply for any other regulator.

We're trying to not specifically look for that by making supplies look
like regular consumers which we were handling by holding a reference on
the module (far from bulletproof but hey).  Special casing them was way
too much of a source of bugs to be sustainable.

> > This seems like storing up trouble for the future, we'll end up with
> > live child devices with parents that weren't around or being refcounted
> > through some of the lifetime of the device which will doubtless come
> > back and bite us later.

> Yes I am not completely happy. Maybe we should wait for the parent
> device to be bound before allowing any of its's regulators to be added
> as supplies for other regulators? I gave the following a quick test and
> this seems to work too ...

Yeah, that thought occurred to me too.  

> The above prevents anyone from using a regulator as a supply if the
> parent has not been bound yet. Therefore, if one of the parent's
> regulators fails to register, after some have already been successfully
> registered, there is no chance any of the successfully added regulators
> will have been already added as a supply to some other regulator in the
> system. Hope that's clear!

Can you cook it up into a proper patch please?  I *think* that should be
OK from a correctness point of view and it should be way more robust (as
well as simpler to implement).

> > Please don't mix different changes into one commit, as covered in
> > SubmittingPatches please send one patch per change.  This makes things
> > easier to 

> Sorry, may be I worded this badly, however, it is a consequence of this
> particular change and otherwise it would not be needed.

Ah, OK.  That makes more sense - at a thousand foot view it wasn't
obvious.

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