[PATCH 1/2] mfd: core: Add the option to order destruction of MFD cells

[PATCH 1/2] mfd: core: Add the option to order destruction of MFD cells

[Charles Keepax]

Sometimes MFD children will have interdependancies. For example an MFD
device might contain a regulator cell and another cell which requires
that regulator to function. Probe deferral will ensure that these
devices probe in the correct order, however currently nothing ensures
they are destroyed in the correct order. As such it is possible for a
cell to be destroyed whilst another cell still expects it to exist. For
example the cell mentioned earlier would attempt to do a regulator_put
as part of its own tear-down but the regulator may have already been
destroyed.

This patch add a field remove_level to the mfd_cell data structure which
allows users to group the destruction of the MFD cells. Cells with a
lower remove_level will be destroyed first. This approach also doesn't
alter the destruction order for existing devices, so should be
no-op for devices not explicitly using it.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/mfd-core.c   |   33 +++++++++++++++++++++++++++++----
 include/linux/mfd/core.h |    3 +++
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 2676492..0c12787 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -226,11 +226,19 @@ fail:
 }
 EXPORT_SYMBOL(mfd_add_devices);
 
+struct mfd_remove_data {
+	atomic_t *cnts;
+
+	int level;
+	int next_level;
+};
+
 static int mfd_remove_devices_fn(struct device *dev, void *c)
 {
 	struct platform_device *pdev;
 	const struct mfd_cell *cell;
-	atomic_t **usage_count = c;
+	struct mfd_remove_data *rd = c;
+	atomic_t **usage_count = &rd->cnts;
 
 	if (dev->type != &mfd_dev_type)
 		return 0;
@@ -238,6 +246,15 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
 	pdev = to_platform_device(dev);
 	cell = mfd_get_cell(pdev);
 
+	if (rd->level < cell->remove_level) {
+		if (!rd->next_level || rd->next_level > cell->remove_level)
+			rd->next_level = cell->remove_level;
+
+		return 0;
+	}
+
+	dev_dbg(dev, "Removing from MFD\n");
+
 	/* find the base address of usage_count pointers (for freeing) */
 	if (!*usage_count || (cell->usage_count < *usage_count))
 		*usage_count = cell->usage_count;
@@ -248,10 +265,18 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
 
 void mfd_remove_devices(struct device *parent)
 {
-	atomic_t *cnts = NULL;
+	struct mfd_remove_data rd = {};
+
+	do {
+		rd.level = rd.next_level;
+		rd.next_level = 0;
+
+		dev_dbg(parent, "Running remove for level: %d\n", rd.level);
+
+		device_for_each_child(parent, &rd, mfd_remove_devices_fn);
+	} while (rd.next_level);
 
-	device_for_each_child(parent, &cnts, mfd_remove_devices_fn);
-	kfree(cnts);
+	kfree(rd.cnts);
 }
 EXPORT_SYMBOL(mfd_remove_devices);
 
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index bdba8c6..40586a6 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -65,6 +65,9 @@ struct mfd_cell {
 	 */
 	const char		**parent_supplies;
 	int			num_parent_supplies;
+
+	/* Cells with a lower remove level will be destroyed first */
+	int			remove_level;
 };
 
 /*
-- 
1.7.2.5

[PATCH 2/2] mfd: arizona: Destroy regulators after the other MFD cells

[Charles Keepax]

Several of the cells depend on the regulators provided by the
arizona-micsupp and arizona-ldo1 cells. As such use the new remove_level
feature of the MFD core to ensure the regulators are destroyed after all
the other cells.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 51c0110..76248bc 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -570,7 +570,10 @@ static inline int arizona_of_get_core_pdata(struct arizona *arizona)
 #endif
 
 static const struct mfd_cell early_devs[] = {
-	{ .name = "arizona-ldo1" },
+	{
+		.name = "arizona-ldo1",
+		.remove_level = 1,
+	},
 };
 
 static const char *wm5102_supplies[] = {
@@ -583,7 +586,10 @@ static const char *wm5102_supplies[] = {
 };
 
 static const struct mfd_cell wm5102_devs[] = {
-	{ .name = "arizona-micsupp" },
+	{
+		.name = "arizona-micsupp",
+		.remove_level = 1,
+	},
 	{ .name = "arizona-extcon" },
 	{ .name = "arizona-gpio" },
 	{ .name = "arizona-haptics" },
@@ -596,7 +602,10 @@ static const struct mfd_cell wm5102_devs[] = {
 };
 
 static const struct mfd_cell wm5110_devs[] = {
-	{ .name = "arizona-micsupp" },
+	{
+		.name = "arizona-micsupp",
+		.remove_level = 1,
+	},
 	{ .name = "arizona-extcon" },
 	{ .name = "arizona-gpio" },
 	{ .name = "arizona-haptics" },
@@ -615,7 +624,10 @@ static const char *wm8997_supplies[] = {
 };
 
 static const struct mfd_cell wm8997_devs[] = {
-	{ .name = "arizona-micsupp" },
+	{
+		.name = "arizona-micsupp",
+		.remove_level = 1,
+	},
 	{ .name = "arizona-extcon" },
 	{ .name = "arizona-gpio" },
 	{ .name = "arizona-haptics" },
-- 
1.7.2.5

Re: [PATCH 1/2] mfd: core: Add the option to order destruction of MFD cells

[Mark Brown]

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

On Mon, Jun 02, 2014 at 10:01:43AM +0100, Charles Keepax wrote:
> Sometimes MFD children will have interdependancies. For example an MFD
> device might contain a regulator cell and another cell which requires
> that regulator to function. Probe deferral will ensure that these
> devices probe in the correct order, however currently nothing ensures
> they are destroyed in the correct order. As such it is possible for a
> cell to be destroyed whilst another cell still expects it to exist. For
> example the cell mentioned earlier would attempt to do a regulator_put
> as part of its own tear-down but the regulator may have already been
> destroyed.

Probe deferral is supposed to handle removal too, we're supposed to be
able to walk the device list in reverse order and everything just work.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] mfd: core: Add the option to order destruction of MFD cells

[Charles Keepax]

On Mon, Jun 02, 2014 at 10:01:17PM +0100, Mark Brown wrote:
> On Mon, Jun 02, 2014 at 10:01:43AM +0100, Charles Keepax wrote:
> > Sometimes MFD children will have interdependancies. For example an MFD
> > device might contain a regulator cell and another cell which requires
> > that regulator to function. Probe deferral will ensure that these
> > devices probe in the correct order, however currently nothing ensures
> > they are destroyed in the correct order. As such it is possible for a
> > cell to be destroyed whilst another cell still expects it to exist. For
> > example the cell mentioned earlier would attempt to do a regulator_put
> > as part of its own tear-down but the regulator may have already been
> > destroyed.
> 
> Probe deferral is supposed to handle removal too, we're supposed to be
> able to walk the device list in reverse order and everything just work.

I had considered this approach but was perhaps incorrectly too
nervous about it. I was slightly concerned about breaking other
MFD devices by changing the order things destroy in. Also the way
the child devices are iterated with device_for_each_child, there is
lack of helpers to process the klist in reverse and it felt like
code I probably shouldn't be modifying.

I am happy to do a version that removes devices in reverse probe
order, if that is preferrable? But any pointers if I am missing
the obvious way to do that would be appreciated.

Thanks,
Charles

Re: [PATCH 1/2] mfd: core: Add the option to order destruction of MFD cells

[Mark Brown]

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

On Tue, Jun 03, 2014 at 10:49:32AM +0100, Charles Keepax wrote:
> On Mon, Jun 02, 2014 at 10:01:17PM +0100, Mark Brown wrote:

> > Probe deferral is supposed to handle removal too, we're supposed to be
> > able to walk the device list in reverse order and everything just work.

> I had considered this approach but was perhaps incorrectly too
> nervous about it. I was slightly concerned about breaking other
> MFD devices by changing the order things destroy in. Also the way

It seems vastly more likely that there's other drivers out there with
poorly tested removal paths than drivers relying on the current
behaviour.

> the child devices are iterated with device_for_each_child, there is
> lack of helpers to process the klist in reverse and it felt like
> code I probably shouldn't be modifying.

> I am happy to do a version that removes devices in reverse probe
> order, if that is preferrable? But any pointers if I am missing
> the obvious way to do that would be appreciated.

You probably need to write the helpers but I'd expect they should be
relatively straightforward.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] mfd: arizona: Destroy regulators after the other MFD cells

[Lee Jones]

On Mon, 02 Jun 2014, Charles Keepax wrote:

> Several of the cells depend on the regulators provided by the
> arizona-micsupp and arizona-ldo1 cells. As such use the new remove_level
> feature of the MFD core to ensure the regulators are destroyed after all
> the other cells.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 51c0110..76248bc 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -570,7 +570,10 @@ static inline int arizona_of_get_core_pdata(struct arizona *arizona)
>  #endif
>  
>  static const struct mfd_cell early_devs[] = {
> -	{ .name = "arizona-ldo1" },
> +	{
> +		.name = "arizona-ldo1",
> +		.remove_level = 1,
> +	},
>  };

Not keen on this approach at all.

+1 for the new reverse-probe-helpers.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: arizona: Destroy regulators after the other MFD cells

[Charles Keepax]

On Tue, Jun 17, 2014 at 03:54:20PM +0100, Lee Jones wrote:
> On Mon, 02 Jun 2014, Charles Keepax wrote:
> 
> > Several of the cells depend on the regulators provided by the
> > arizona-micsupp and arizona-ldo1 cells. As such use the new remove_level
> > feature of the MFD core to ensure the regulators are destroyed after all
> > the other cells.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/mfd/arizona-core.c |   20 ++++++++++++++++----
> >  1 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> > index 51c0110..76248bc 100644
> > --- a/drivers/mfd/arizona-core.c
> > +++ b/drivers/mfd/arizona-core.c
> > @@ -570,7 +570,10 @@ static inline int arizona_of_get_core_pdata(struct arizona *arizona)
> >  #endif
> >  
> >  static const struct mfd_cell early_devs[] = {
> > -	{ .name = "arizona-ldo1" },
> > +	{
> > +		.name = "arizona-ldo1",
> > +		.remove_level = 1,
> > +	},
> >  };
> 
> Not keen on this approach at all.
> 
> +1 for the new reverse-probe-helpers.

Yeah that does seem like a better solution, I am looking at it
but it is a little more complex than I hoped so will be a
slight delay on getting a patch out for it.

Thanks,
Charles