[PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[Charles Keepax]

The current unbinding process for Madera has some issues. The trouble
is runtime PM is disabled as the first step of the process, but
some of the drivers release IRQs causing regmap IRQ to issue a
runtime get which fails. To allow runtime PM to remain enabled during
mfd_remove_devices, the DCVDD regulator must remain available. In
the case of external DCVDD's this is simple, the regulator can simply
be disabled/put after the call to mfd_remove_devices. However, in
the case of an internally supplied DCVDD the regulator needs to be
released after all the MFD children, except for the regulator child
itself, have been removed. This is achieved by having the regulator
driver itself do the disable/put, as it is the last driver removed from
the MFD.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/regulator/arizona-ldo1.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index 1a3d7b720f5e0..83dd37dbfe07b 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -375,6 +375,18 @@ static int madera_ldo1_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int madera_ldo1_remove(struct platform_device *pdev)
+{
+	struct madera *madera = dev_get_drvdata(pdev->dev.parent);
+
+	if (madera->internal_dcvdd) {
+		regulator_disable(madera->dcvdd);
+		regulator_put(madera->dcvdd);
+	}
+
+	return arizona_ldo1_remove(pdev);
+}
+
 static struct platform_driver arizona_ldo1_driver = {
 	.probe = arizona_ldo1_probe,
 	.remove = arizona_ldo1_remove,
@@ -385,7 +397,7 @@ static struct platform_driver arizona_ldo1_driver = {
 
 static struct platform_driver madera_ldo1_driver = {
 	.probe = madera_ldo1_probe,
-	.remove = arizona_ldo1_remove,
+	.remove = madera_ldo1_remove,
 	.driver		= {
 		.name	= "madera-ldo1",
 	},
-- 
2.11.0

[PATCH RESEND 2/2] mfd: madera: Improve handling of regulator unbinding

[Charles Keepax]

The current unbinding process for Madera has some issues. The trouble
is runtime PM is disabled as the first step of the process, but
some of the drivers release IRQs causing regmap IRQ to issue a
runtime get which fails. To allow runtime PM to remain enabled during
mfd_remove_devices, the DCVDD regulator must remain available. In
the case of external DCVDD's this is simple, the regulator can simply
be disabled/put after the call to mfd_remove_devices. However, in
the case of an internally supplied DCVDD the regulator needs to be
released after all the MFD children, except for the regulator child
itself, have been removed. This is achieved by having the regulator
driver itself do the disable/put, as it is the last driver removed from
the MFD.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/mfd/madera-core.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index a8cfadc1fc01e..5170b55836fc1 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -730,18 +730,22 @@ int madera_dev_exit(struct madera *madera)
 	/* Prevent any IRQs being serviced while we clean up */
 	disable_irq(madera->irq);
 
-	/*
-	 * DCVDD could be supplied by a child node, we must disable it before
-	 * removing the children, and prevent PM runtime from turning it back on
-	 */
+	pm_runtime_get_sync(madera->dev);
+
+	mfd_remove_devices(madera->dev);
+
 	pm_runtime_disable(madera->dev);
 
-	clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+	if (!madera->internal_dcvdd) {
+		regulator_disable(madera->dcvdd);
+		regulator_put(madera->dcvdd);
+	}
 
-	regulator_disable(madera->dcvdd);
-	regulator_put(madera->dcvdd);
+	pm_runtime_set_suspended(madera->dev);
+	pm_runtime_put_noidle(madera->dev);
+
+	clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
 
-	mfd_remove_devices(madera->dev);
 	madera_enable_hard_reset(madera);
 
 	regulator_bulk_disable(madera->num_core_supplies,
-- 
2.11.0

Re: [PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[Mark Brown]

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

On Wed, Jan 22, 2020 at 11:08:41AM +0000, Charles Keepax wrote:

> The current unbinding process for Madera has some issues. The trouble
> is runtime PM is disabled as the first step of the process, but

Why not just leave runtime PM active until all the subdevices are gone?
This is a really bad hack and it's going to be fragile.

> +static int madera_ldo1_remove(struct platform_device *pdev)
> +{
> +	struct madera *madera = dev_get_drvdata(pdev->dev.parent);
> +
> +	if (madera->internal_dcvdd) {
> +		regulator_disable(madera->dcvdd);
> +		regulator_put(madera->dcvdd);
> +	}

This is going to break bisection since it will result in double
disables, it'd be fine to do the MFD change first since that'd just
leak a reference to enable on a regulator which is about to be discarded
entirely anyway but this reordering (and whatever other changes you've
done since v1) means you add a double free.

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

Re: [PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[Charles Keepax]

On Wed, Jan 22, 2020 at 01:11:49PM +0000, Mark Brown wrote:
> On Wed, Jan 22, 2020 at 11:08:41AM +0000, Charles Keepax wrote:
> 
> > The current unbinding process for Madera has some issues. The trouble
> > is runtime PM is disabled as the first step of the process, but
> 
> Why not just leave runtime PM active until all the subdevices are gone?
> This is a really bad hack and it's going to be fragile.
> 

Admittedly I am not super fond of this solution either. But
leaving the PM runtime active is basically what this patch does
(well the mfd part). Leaving the PM runtime enabled means access
to the DCVDD regulator is required during the remove process,
which in turn means you need to put that regulator after the
other devices are removed but before DCVDD is removed. Currently
the only place we can do that is in the LDO remove, as per this
patch.

Other options that might be viable, pending input for yourself
and Lee:

1) We could look at adding a partial remove function to MFD.
Currently I can only call mfd_remove_devices which nobbles all
the devices. If I could make calls to remove specific devices I
could do one call to remove everything except DCVDD, do the put,
then remove the regulator.

2) We could look at adding some sort of pre-remove callback into
MFD, and the regulator put could go in there rather than the
regulator remove, as per this patch. Although this feels a little
like the same thing as this patch, just dressed up a little
differently.

3) We could look at doing something in regmap IRQ to change when
it does PM runtime calls, it is regmap doing runtime gets when
drivers remove IRQs that causes the issue. But my accessment was
that what regmap is doing makes perfect sense, so I don't think
this is a good approach.

> > +static int madera_ldo1_remove(struct platform_device *pdev)
> > +{
> > +	struct madera *madera = dev_get_drvdata(pdev->dev.parent);
> > +
> > +	if (madera->internal_dcvdd) {
> > +		regulator_disable(madera->dcvdd);
> > +		regulator_put(madera->dcvdd);
> > +	}
> 
> This is going to break bisection since it will result in double
> disables, it'd be fine to do the MFD change first since that'd just
> leak a reference to enable on a regulator which is about to be discarded
> entirely anyway but this reordering (and whatever other changes you've
> done since v1) means you add a double free.

Apologies yes that is a good point. If no one minds, and we end
up sticking with this approach, I feel it might best to squash
them both back into one patch?

Thanks,
Charles

Re: [PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[Mark Brown]

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

On Thu, Jan 23, 2020 at 09:26:39AM +0000, Charles Keepax wrote:

> 3) We could look at doing something in regmap IRQ to change when
> it does PM runtime calls, it is regmap doing runtime gets when
> drivers remove IRQs that causes the issue. But my accessment was
> that what regmap is doing makes perfect sense, so I don't think
> this is a good approach.

Why do you even care about the errors?  It's not like this device is
going to get removed in a production system and the primary IRQ will be
disabled when the core is removed, this is just something that happens
during development isn't it?

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

Re: [PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[Charles Keepax]

On Thu, Jan 23, 2020 at 11:48:05AM +0000, Mark Brown wrote:
> On Thu, Jan 23, 2020 at 09:26:39AM +0000, Charles Keepax wrote:
> 
> > 3) We could look at doing something in regmap IRQ to change when
> > it does PM runtime calls, it is regmap doing runtime gets when
> > drivers remove IRQs that causes the issue. But my accessment was
> > that what regmap is doing makes perfect sense, so I don't think
> > this is a good approach.
> 
> Why do you even care about the errors?  It's not like this device is
> going to get removed in a production system and the primary IRQ will be
> disabled when the core is removed, this is just something that happens
> during development isn't it?

We do have certain customers who test unbinding the driver and
complain when it throws errors. Admittedly, you are correct that
it is a little bit of a stretch to imagine a situation where this
is a massive problem in production. Best I can offer is one of
our CODECs gets into a laptop and someone wants to unbind/bind
the driver, to clear some issue or something.

Its been a while since I was looking at this issue, so I will
double check to see if the issue is purely one of error messages
or if it actually can cause problems (memory leaks or issues
rebinding). But on a personal note I would be somewhat happier if
we could come up with some acceptable way to make the driver
unbind cleanly.

I am more than happy to do the leg work if we really don't like
this solution. Do either you or Lee have any thoughts on my
selective MFD remove helpers? That seemed like the most promising
alternative solution to me.

Thanks,
Charles

Re: [PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[Mark Brown]

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

On Thu, Jan 23, 2020 at 12:02:40PM +0000, Charles Keepax wrote:

> I am more than happy to do the leg work if we really don't like
> this solution. Do either you or Lee have any thoughts on my
> selective MFD remove helpers? That seemed like the most promising
> alternative solution to me.

That's my first thought if you need to control removal order which is
basically what's going on here.

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

Re: [PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of regulator unbinding

[Lee Jones]

On Thu, 23 Jan 2020, Charles Keepax wrote:

> On Thu, Jan 23, 2020 at 11:48:05AM +0000, Mark Brown wrote:
> > On Thu, Jan 23, 2020 at 09:26:39AM +0000, Charles Keepax wrote:
> > 
> > > 3) We could look at doing something in regmap IRQ to change when
> > > it does PM runtime calls, it is regmap doing runtime gets when
> > > drivers remove IRQs that causes the issue. But my accessment was
> > > that what regmap is doing makes perfect sense, so I don't think
> > > this is a good approach.
> > 
> > Why do you even care about the errors?  It's not like this device is
> > going to get removed in a production system and the primary IRQ will be
> > disabled when the core is removed, this is just something that happens
> > during development isn't it?
> 
> I am more than happy to do the leg work if we really don't like
> this solution. Do either you or Lee have any thoughts on my
> selective MFD remove helpers? That seemed like the most promising
> alternative solution to me.

It's hard to say without seeing your implementation, but it sounds
okay in principle.  Depends how messy it all ends up getting.  Sounds
like a scenario where a reverse -EDEFER_PROBE could be useful.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog