[PATCH 0/3] Fix WARN_ON caused by "mfd: Allow mapping regulator supplies to MFD device from children"

[PATCH 0/3] Fix WARN_ON caused by "mfd: Allow mapping regulator supplies to MFD device from children"

[Hans de Goede]

Hi all,

I hit this WARN_ON:

WARNING: CPU: 0 PID: 1 at drivers/base/dd.c:286 driver_probe_device...
Which points to this line:

	WARN_ON(!list_empty(&dev->devres_head));

While testing Carlo Caione's axp20x regulator patches on top of
3.15-rc5. The problem is that mfd_add_device() from drivers/mfd/mfd-core.c
makes a call to devm_regulator_bulk_register_supply_alias() before doing the
platform_device_add, which results in dev->devres_head not being empty,
triggering the warning.

The code triggering this was introduced by this commit:
"mfd: Allow mapping regulator supplies to MFD device from children" :
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7fcd4274

Which puts the registering of the aliases before the
platform_device_add, so lets try moving it to below the platform_device_add.

This changes the messages about the alias addition from:

[    1.386371] Adding alias for supply acin,(null) -> acin,0-0034
[    1.392205] Adding alias for supply vin2,(null) -> vin2,0-0034
[    1.398112] Adding alias for supply vin3,(null) -> vin3,0-0034
[    1.403995] Adding alias for supply ldo24in,(null) -> ldo24in,0-0034
[    1.410344] Adding alias for supply ldo3in,(null) -> ldo3in,0-0034
[    1.416551] Adding alias for supply ldo5in,(null) -> ldo5in,0-0034

To:

[    1.410545] Adding alias for supply acin,axp20x-regulator -> acin,0-0034
[    1.417288] Adding alias for supply vin2,axp20x-regulator -> vin2,0-0034
[    1.424002] Adding alias for supply vin3,axp20x-regulator -> vin3,0-0034
[    1.430695] Adding alias for supply ldo24in,axp20x-regulator -> ldo24in,0-003
4
[    1.437922] Adding alias for supply ldo3in,axp20x-regulator -> ldo3in,0-0034
[    1.444973] Adding alias for supply ldo5in,axp20x-regulator -> ldo5in,0-0034

Which looks more correct, but this leads to other problems:

[    1.386241] LDO1: 1300 mV
[    1.388989] axp20x-regulator axp20x-regulator: Failed to find supply acin
[    1.395978] axp20x-regulator axp20x-regulator: Failed to register LDO1
[    1.402513] platform axp20x-regulator: Driver axp20x-regulator requests probe
 deferral

Followed by:
[    2.191834] WARNING: CPU: 0 PID: 6 at drivers/base/dd.c:286 driver_probe_devi
ce+0xe0/0x344()
[    2.200323] Modules linked in:
[    2.203422] CPU: 0 PID: 6 Comm: kworker/u4:0 Not tainted 3.15.0-rc5+ #143
[    2.210209] Workqueue: deferwq deferred_probe_work_func

So the exact same problem, what happens here is that the driver probe gets deferred,
then we add the aliases, and then when the deferred probe runs we're in the
same situation again that the devres list is not empty at probe time.

Looking more into this, I also noticed that mfd_add_device also has a
call to devm_regulator_bulk_unregister_supply_alias(), which is weird because
the whole idea of devm functions is that they cleanup after themselves.

All this together has led me to the conclusion that the current approach is
simply wrong, and that the regulator alias registering should be done in
the platform drivers probe method. 

So that is what this patch set does. Note I've kept most of the related code
the same. Esp. including the listing of the parent supplies in the cell info,
as I feel that that is the right place for it.

Regards,

Hans

[PATCH 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Hans de Goede]

We cannot register supply alias in mfd_add_device before calling
platform_add_device, for 2 reasons:
1) devm resources may not be registered before the (platform) drivers probe
   method runs
2) The platform-dev's name must be set before registering the aliases which
   happens from platform_add_device.

So stop registering supply aliases from mfd_add_device, and add a
mfd_register_supply_aliases helper functions for the cell's plaform driver
probe method to use.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
 include/linux/mfd/core.h |  6 +++++-
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 2676492..d1f929a 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -102,13 +102,6 @@ static int mfd_add_device(struct device *parent, int id,
 	pdev->dev.dma_mask = parent->dma_mask;
 	pdev->dev.dma_parms = parent->dma_parms;
 
-	ret = devm_regulator_bulk_register_supply_alias(
-			&pdev->dev, cell->parent_supplies,
-			parent, cell->parent_supplies,
-			cell->num_parent_supplies);
-	if (ret < 0)
-		goto fail_res;
-
 	if (parent->of_node && cell->of_compatible) {
 		for_each_child_of_node(parent->of_node, np) {
 			if (of_device_is_compatible(np, cell->of_compatible)) {
@@ -122,12 +115,12 @@ static int mfd_add_device(struct device *parent, int id,
 		ret = platform_device_add_data(pdev,
 					cell->platform_data, cell->pdata_size);
 		if (ret)
-			goto fail_alias;
+			goto fail_res;
 	}
 
 	ret = mfd_platform_add_cell(pdev, cell, usage_count);
 	if (ret)
-		goto fail_alias;
+		goto fail_res;
 
 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;
@@ -162,17 +155,17 @@ static int mfd_add_device(struct device *parent, int id,
 		if (!cell->ignore_resource_conflicts) {
 			ret = acpi_check_resource_conflict(&res[r]);
 			if (ret)
-				goto fail_alias;
+				goto fail_res;
 		}
 	}
 
 	ret = platform_device_add_resources(pdev, res, cell->num_resources);
 	if (ret)
-		goto fail_alias;
+		goto fail_res;
 
 	ret = platform_device_add(pdev);
 	if (ret)
-		goto fail_alias;
+		goto fail_res;
 
 	if (cell->pm_runtime_no_callbacks)
 		pm_runtime_no_callbacks(&pdev->dev);
@@ -181,10 +174,6 @@ static int mfd_add_device(struct device *parent, int id,
 
 	return 0;
 
-fail_alias:
-	devm_regulator_bulk_unregister_supply_alias(&pdev->dev,
-						    cell->parent_supplies,
-						    cell->num_parent_supplies);
 fail_res:
 	kfree(res);
 fail_device:
@@ -286,5 +275,21 @@ int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones)
 }
 EXPORT_SYMBOL(mfd_clone_cell);
 
+int mfd_register_supply_aliases(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell;
+
+	if (pdev->dev.type != &mfd_dev_type)
+		return -EINVAL;
+
+	cell = mfd_get_cell(pdev);
+
+	return devm_regulator_bulk_register_supply_alias(
+			&pdev->dev, cell->parent_supplies,
+			pdev->dev.parent, cell->parent_supplies,
+			cell->num_parent_supplies);
+}
+EXPORT_SYMBOL(mfd_register_supply_aliases);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Ian Molton, Dmitry Baryshkov");
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index bdba8c6..a467307 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -61,7 +61,9 @@ struct mfd_cell {
 	bool			pm_runtime_no_callbacks;
 
 	/* A list of regulator supplies that should be mapped to the MFD
-	 * device rather than the child device when requested
+	 * device rather than the child device when requested.
+	 * Drivers using this must call mfd_register_supply_aliases()
+	 * from their probe method.
 	 */
 	const char		**parent_supplies;
 	int			num_parent_supplies;
@@ -110,4 +112,6 @@ extern int mfd_add_devices(struct device *parent, int id,
 
 extern void mfd_remove_devices(struct device *parent);
 
+extern int mfd_register_supply_aliases(struct platform_device *pdev);
+
 #endif
-- 
1.9.0

[PATCH 2/3] regulator/axp20x: Call mfd_register_supply_aliases

[Hans de Goede]

The mfd-core no longer registers the supply aliases, instead the platform
driver probe method must now call mfd_register_supply_aliases().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/regulator/axp20x-regulator.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 78a29e6..ff35758 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/mfd/axp20x.h>
+#include <linux/mfd/core.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
 
@@ -237,6 +238,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 	int ret, i;
 	u32 workmode;
 
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
+
 	ret = axp20x_regulator_parse_dt(pdev);
 	if (ret)
 		return ret;
-- 
1.9.0

[PATCH 3/3] arizona-mfd-codecs: Add mfd_register_supply_aliases() calls

[Hans de Goede]

The mfd-core no longer registers the supply aliases, instead the platform
driver probe method must now call mfd_register_supply_aliases().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/wm5102.c | 5 +++++
 sound/soc/codecs/wm5110.c | 5 +++++
 sound/soc/codecs/wm8997.c | 7 ++++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c
index dcf1d12..fab08cc 100644
--- a/sound/soc/codecs/wm5102.c
+++ b/sound/soc/codecs/wm5102.c
@@ -26,6 +26,7 @@
 #include <sound/initval.h>
 #include <sound/tlv.h>
 
+#include <linux/mfd/core.h>
 #include <linux/mfd/arizona/core.h>
 #include <linux/mfd/arizona/registers.h>
 
@@ -1825,6 +1826,10 @@ static int wm5102_probe(struct platform_device *pdev)
 	struct wm5102_priv *wm5102;
 	int i, ret;
 
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
+
 	wm5102 = devm_kzalloc(&pdev->dev, sizeof(struct wm5102_priv),
 			      GFP_KERNEL);
 	if (wm5102 == NULL)
diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c
index df5a38d..a1a6923 100644
--- a/sound/soc/codecs/wm5110.c
+++ b/sound/soc/codecs/wm5110.c
@@ -26,6 +26,7 @@
 #include <sound/initval.h>
 #include <sound/tlv.h>
 
+#include <linux/mfd/core.h>
 #include <linux/mfd/arizona/core.h>
 #include <linux/mfd/arizona/registers.h>
 
@@ -1656,6 +1657,10 @@ static int wm5110_probe(struct platform_device *pdev)
 	struct wm5110_priv *wm5110;
 	int i, ret;
 
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
+
 	wm5110 = devm_kzalloc(&pdev->dev, sizeof(struct wm5110_priv),
 			      GFP_KERNEL);
 	if (wm5110 == NULL)
diff --git a/sound/soc/codecs/wm8997.c b/sound/soc/codecs/wm8997.c
index 004186b..099b148 100644
--- a/sound/soc/codecs/wm8997.c
+++ b/sound/soc/codecs/wm8997.c
@@ -26,6 +26,7 @@
 #include <sound/initval.h>
 #include <sound/tlv.h>
 
+#include <linux/mfd/core.h>
 #include <linux/mfd/arizona/core.h>
 #include <linux/mfd/arizona/registers.h>
 
@@ -1107,7 +1108,11 @@ static int wm8997_probe(struct platform_device *pdev)
 {
 	struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
 	struct wm8997_priv *wm8997;
-	int i;
+	int i, ret;
+
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
 
 	wm8997 = devm_kzalloc(&pdev->dev, sizeof(struct wm8997_priv),
 			      GFP_KERNEL);
-- 
1.9.0

Re: [PATCH 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Lee Jones]

> We cannot register supply alias in mfd_add_device before calling
> platform_add_device, for 2 reasons:
> 1) devm resources may not be registered before the (platform) drivers probe
>    method runs
> 2) The platform-dev's name must be set before registering the aliases which
>    happens from platform_add_device.
> 
> So stop registering supply aliases from mfd_add_device, and add a
> mfd_register_supply_aliases helper functions for the cell's plaform driver
> probe method to use.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
>  include/linux/mfd/core.h |  6 +++++-
>  2 files changed, 26 insertions(+), 17 deletions(-)

Change looks reasonable to me, but I'd like to have Mark look over the
changes.  If he's okay with them I think it's best for this set to go
through the MFD tree as a whole.

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

Re: [PATCH 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Lee Jones]

> > We cannot register supply alias in mfd_add_device before calling
> > platform_add_device, for 2 reasons:
> > 1) devm resources may not be registered before the (platform) drivers probe
> >    method runs
> > 2) The platform-dev's name must be set before registering the aliases which
> >    happens from platform_add_device.
> > 
> > So stop registering supply aliases from mfd_add_device, and add a
> > mfd_register_supply_aliases helper functions for the cell's plaform driver
> > probe method to use.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
> >  include/linux/mfd/core.h |  6 +++++-
> >  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> Change looks reasonable to me, but I'd like to have Mark look over the
> changes.  If he's okay with them I think it's best for this set to go
> through the MFD tree as a whole.

For my own benefit, rather than a go-ahead for Mark to apply them: ;)

  Acked-by: Lee Jones <lee.jones@linaro.org>

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

Re: [PATCH 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Charles Keepax]

On Mon, May 19, 2014 at 08:47:05AM +0100, Lee Jones wrote:
> > We cannot register supply alias in mfd_add_device before calling
> > platform_add_device, for 2 reasons:
> > 1) devm resources may not be registered before the (platform) drivers probe
> >    method runs
> > 2) The platform-dev's name must be set before registering the aliases which
> >    happens from platform_add_device.
> > 
> > So stop registering supply aliases from mfd_add_device, and add a
> > mfd_register_supply_aliases helper functions for the cell's plaform driver
> > probe method to use.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
> >  include/linux/mfd/core.h |  6 +++++-
> >  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> Change looks reasonable to me, but I'd like to have Mark look over the
> changes.  If he's okay with them I think it's best for this set to go
> through the MFD tree as a whole.

This should already be fixed by this patch:

mfd: core: Don't use devres functions before device is added

You had posted to say it was applied although I haven't checked
your tree for it directly.

Thanks,
Charles

Re: [PATCH 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Hans de Goede]

Hi,

On 05/19/2014 09:48 AM, Charles Keepax wrote:
> On Mon, May 19, 2014 at 08:47:05AM +0100, Lee Jones wrote:
>>> We cannot register supply alias in mfd_add_device before calling
>>> platform_add_device, for 2 reasons:
>>> 1) devm resources may not be registered before the (platform) drivers probe
>>>    method runs
>>> 2) The platform-dev's name must be set before registering the aliases which
>>>    happens from platform_add_device.
>>>
>>> So stop registering supply aliases from mfd_add_device, and add a
>>> mfd_register_supply_aliases helper functions for the cell's plaform driver
>>> probe method to use.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
>>>  include/linux/mfd/core.h |  6 +++++-
>>>  2 files changed, 26 insertions(+), 17 deletions(-)
>>
>> Change looks reasonable to me, but I'd like to have Mark look over the
>> changes.  If he's okay with them I think it's best for this set to go
>> through the MFD tree as a whole.
> 
> This should already be fixed by this patch:
> 
> mfd: core: Don't use devres functions before device is added

Ah, I did not check next, yes that would fix *half* of the problem,
the other half is that adding aliases uses dev->name (at least for
logging, did not check if it is used for anything else) and that is
not set yet before the device is added.

Basically the moral of the story is that it is a BAD idea to do
anything with a device before it is added. So my proposed series
would replace the "mfd: core: Don't use devres functions before device is added"
patch.

Regards,

Hans

Re: [PATCH 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Charles Keepax]

On Mon, May 19, 2014 at 10:18:11AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 05/19/2014 09:48 AM, Charles Keepax wrote:
> > On Mon, May 19, 2014 at 08:47:05AM +0100, Lee Jones wrote:
> >>> We cannot register supply alias in mfd_add_device before calling
> >>> platform_add_device, for 2 reasons:
> >>> 1) devm resources may not be registered before the (platform) drivers probe
> >>>    method runs
> >>> 2) The platform-dev's name must be set before registering the aliases which
> >>>    happens from platform_add_device.
> >>>
> >>> So stop registering supply aliases from mfd_add_device, and add a
> >>> mfd_register_supply_aliases helper functions for the cell's plaform driver
> >>> probe method to use.
> >>>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>>  drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
> >>>  include/linux/mfd/core.h |  6 +++++-
> >>>  2 files changed, 26 insertions(+), 17 deletions(-)
> >>
> >> Change looks reasonable to me, but I'd like to have Mark look over the
> >> changes.  If he's okay with them I think it's best for this set to go
> >> through the MFD tree as a whole.
> > 
> > This should already be fixed by this patch:
> > 
> > mfd: core: Don't use devres functions before device is added
> 
> Ah, I did not check next, yes that would fix *half* of the problem,
> the other half is that adding aliases uses dev->name (at least for
> logging, did not check if it is used for anything else) and that is
> not set yet before the device is added.
> 
> Basically the moral of the story is that it is a BAD idea to do
> anything with a device before it is added. So my proposed series
> would replace the "mfd: core: Don't use devres functions before device is added"
> patch.
> 
> Regards,
> 
> Hans

Cool cool, since my patch doesn't seem to have made it in anyway
I guess lets proceed with your series then.

Thanks,
Charles

Re: [PATCH 3/3] arizona-mfd-codecs: Add mfd_register_supply_aliases() calls

[Charles Keepax]

On Sun, May 18, 2014 at 03:49:13PM +0200, Hans de Goede wrote:
> The mfd-core no longer registers the supply aliases, instead the platform
> driver probe method must now call mfd_register_supply_aliases().
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles

Re: [PATCH 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Lee Jones]

> > >>> We cannot register supply alias in mfd_add_device before calling
> > >>> platform_add_device, for 2 reasons:
> > >>> 1) devm resources may not be registered before the (platform) drivers probe
> > >>>    method runs
> > >>> 2) The platform-dev's name must be set before registering the aliases which
> > >>>    happens from platform_add_device.
> > >>>
> > >>> So stop registering supply aliases from mfd_add_device, and add a
> > >>> mfd_register_supply_aliases helper functions for the cell's plaform driver
> > >>> probe method to use.
> > >>>
> > >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >>> ---
> > >>>  drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
> > >>>  include/linux/mfd/core.h |  6 +++++-
> > >>>  2 files changed, 26 insertions(+), 17 deletions(-)
> > >>
> > >> Change looks reasonable to me, but I'd like to have Mark look over the
> > >> changes.  If he's okay with them I think it's best for this set to go
> > >> through the MFD tree as a whole.
> > > 
> > > This should already be fixed by this patch:
> > > 
> > > mfd: core: Don't use devres functions before device is added
> > 
> > Ah, I did not check next, yes that would fix *half* of the problem,
> > the other half is that adding aliases uses dev->name (at least for
> > logging, did not check if it is used for anything else) and that is
> > not set yet before the device is added.
> > 
> > Basically the moral of the story is that it is a BAD idea to do
> > anything with a device before it is added. So my proposed series
> > would replace the "mfd: core: Don't use devres functions before device is added"
> > patch.
> 
> Cool cool, since my patch doesn't seem to have made it in anyway
> I guess lets proceed with your series then.

The patch is applied locally, but the branch isn't pushed.  I'll
replace that patch with this set - once Mark provides his feedback.

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