[PATCH 0/5] mmc: cavium: bug fixes for 4.12

[PATCH 0/5] mmc: cavium: bug fixes for 4.12

[Jan Glauber]

Hi Ulf,

here are some bug fixes for the new mmc driver. The only
non-trivial fix should be the platform thing in patch #4 and #5.

Tested on OcteonTx and on various MIPS boxes.

thanks,
Jan

David Daney (2):
  mmc: cavium-octeon: Fix interrupt enable code
  mmc: cavium-octeon: Use proper GPIO name for power control

Jan Glauber (3):
  mmc: cavium: Prevent crash with incomplete DT
  of/platform: Make of_platform_device_destroy globally visible
  mmc: cavium: Fix probing race with regulator

 drivers/mmc/host/cavium-octeon.c   | 4 ++--
 drivers/mmc/host/cavium-thunderx.c | 4 +++-
 drivers/mmc/host/cavium.c          | 8 ++++++--
 drivers/of/platform.c              | 3 ++-
 include/linux/of_platform.h        | 1 +
 5 files changed, 14 insertions(+), 6 deletions(-)

-- 
2.9.0.rc0.21.g7777322

[PATCH 1/5] mmc: cavium-octeon: Fix interrupt enable code

[Jan Glauber]

From: David Daney <david.daney@cavium.com>

OCTEON SoCs with CIU3 do not have interrupt masking local to the MMC
bus interface.  Unfortunately, some even have a diagnostic register at
the same address of the enable register, which causes the interrupts
to fire immediately if stored to, thus breaking the driver.  The proper
action on these SoCs is not to touch this register.

Fixes: 01d95843335c ("mmc: cavium: Add MMC support for Octeon SOCs.")
Signed-off-by: David Daney <david.daney@cavium.com>
[jglauber@cavium.com: removed point after subject line]
Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/mmc/host/cavium-octeon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/cavium-octeon.c b/drivers/mmc/host/cavium-octeon.c
index 772d090..d698d66 100644
--- a/drivers/mmc/host/cavium-octeon.c
+++ b/drivers/mmc/host/cavium-octeon.c
@@ -108,7 +108,7 @@ static void octeon_mmc_release_bus(struct cvm_mmc_host *host)
 static void octeon_mmc_int_enable(struct cvm_mmc_host *host, u64 val)
 {
 	writeq(val, host->base + MIO_EMM_INT(host));
-	if (!host->dma_active || (host->dma_active && !host->has_ciu3))
+	if (!host->has_ciu3)
 		writeq(val, host->base + MIO_EMM_INT_EN(host));
 }
 
-- 
2.9.0.rc0.21.g7777322

[PATCH 2/5] mmc: cavium-octeon: Use proper GPIO name for power control

[Jan Glauber]

From: David Daney <david.daney@cavium.com>

The devm_gpiod_get_optional() function appends a "-gpios" to the
string passed to it, so if we want to find the "power-gpios" signal,
we must pass "power" to this function.

Fixes: 01d95843335c ("mmc: cavium: Add MMC support for Octeon SOCs.")
Signed-off-by: David Daney <david.daney@cavium.com>
[jglauber@cavium.com: removed point after subject line]
Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/mmc/host/cavium-octeon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/cavium-octeon.c b/drivers/mmc/host/cavium-octeon.c
index d698d66..cbb5663 100644
--- a/drivers/mmc/host/cavium-octeon.c
+++ b/drivers/mmc/host/cavium-octeon.c
@@ -267,7 +267,7 @@ static int octeon_mmc_probe(struct platform_device *pdev)
 	}
 
 	host->global_pwr_gpiod = devm_gpiod_get_optional(&pdev->dev,
-							 "power-gpios",
+							 "power",
 							 GPIOD_OUT_HIGH);
 	if (IS_ERR(host->global_pwr_gpiod)) {
 		dev_err(&pdev->dev, "Invalid power GPIO\n");
-- 
2.9.0.rc0.21.g7777322

[PATCH 3/5] mmc: cavium: Prevent crash with incomplete DT

[Jan Glauber]

In case the DT specifies neither a regulator nor a gpio
for the shared power the driver will crash accessing the regulator.
Prevent the crash by checking the regulator before use.

As the MMC devices would likely not be usable without power
check for that condition during probe and print a warning.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/mmc/host/cavium.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/cavium.c b/drivers/mmc/host/cavium.c
index 58b51ba..63f96dc 100644
--- a/drivers/mmc/host/cavium.c
+++ b/drivers/mmc/host/cavium.c
@@ -839,14 +839,14 @@ static void cvm_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		cvm_mmc_reset_bus(slot);
 		if (host->global_pwr_gpiod)
 			host->set_shared_power(host, 0);
-		else
+		else if (mmc->supply.vmmc)
 			mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
 		break;
 
 	case MMC_POWER_UP:
 		if (host->global_pwr_gpiod)
 			host->set_shared_power(host, 1);
-		else
+		else if (mmc->supply.vmmc)
 			mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
 		break;
 	}
@@ -976,6 +976,7 @@ static int cvm_mmc_of_parse(struct device *dev, struct cvm_mmc_slot *slot)
 		 * Legacy Octeon firmware has no regulator entry, fall-back to
 		 * a hard-coded voltage to get a sane OCR.
 		 */
+		mmc->supply.vmmc = NULL;
 		mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
 	} else {
 		ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc);
@@ -983,6 +984,9 @@ static int cvm_mmc_of_parse(struct device *dev, struct cvm_mmc_slot *slot)
 			mmc->ocr_avail = ret;
 	}
 
+	if (!mmc->supply.vmmc && !slot->host->global_pwr_gpiod)
+		dev_warn(dev, "Missing power regulator or gpio, device may not work.\n");
+
 	/* Common MMC bindings */
 	ret = mmc_of_parse(mmc);
 	if (ret)
-- 
2.9.0.rc0.21.g7777322

Re: [PATCH 3/5] mmc: cavium: Prevent crash with incomplete DT

[Ulf Hansson]

On 16 May 2017 at 11:36, Jan Glauber <jglauber@cavium.com> wrote:
> In case the DT specifies neither a regulator nor a gpio
> for the shared power the driver will crash accessing the regulator.
> Prevent the crash by checking the regulator before use.
>
> As the MMC devices would likely not be usable without power
> check for that condition during probe and print a warning.
>
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> ---
>  drivers/mmc/host/cavium.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/cavium.c b/drivers/mmc/host/cavium.c
> index 58b51ba..63f96dc 100644
> --- a/drivers/mmc/host/cavium.c
> +++ b/drivers/mmc/host/cavium.c
> @@ -839,14 +839,14 @@ static void cvm_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                 cvm_mmc_reset_bus(slot);
>                 if (host->global_pwr_gpiod)
>                         host->set_shared_power(host, 0);
> -               else
> +               else if (mmc->supply.vmmc)

Please do this instead:

else if (!IS_ERR(mmc->supply.vmmc))

>                         mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
>                 break;
>
>         case MMC_POWER_UP:
>                 if (host->global_pwr_gpiod)
>                         host->set_shared_power(host, 1);
> -               else
> +               else if (mmc->supply.vmmc)

Ditto

>                         mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
>                 break;
>         }
> @@ -976,6 +976,7 @@ static int cvm_mmc_of_parse(struct device *dev, struct cvm_mmc_slot *slot)
>                  * Legacy Octeon firmware has no regulator entry, fall-back to
>                  * a hard-coded voltage to get a sane OCR.
>                  */
> +               mmc->supply.vmmc = NULL;

... and then you shall remove this.

>                 mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
>         } else {
>                 ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc);
> @@ -983,6 +984,9 @@ static int cvm_mmc_of_parse(struct device *dev, struct cvm_mmc_slot *slot)
>                         mmc->ocr_avail = ret;
>         }
>
> +       if (!mmc->supply.vmmc && !slot->host->global_pwr_gpiod)
> +               dev_warn(dev, "Missing power regulator or gpio, device may not work.\n");
> +

While you re-spins this, I suggest you to also convert to use the
mmc_regulator_get_supply() API.
That should simply the code a bit and also avoid you from needing to
print anything like this above, as there are already some printing
being done in mmc_regulator_get_supply().

>         /* Common MMC bindings */
>         ret = mmc_of_parse(mmc);
>         if (ret)
> --
> 2.9.0.rc0.21.g7777322
>

Kind regards
Uffe

[PATCH 4/5] of/platform: Make of_platform_device_destroy globally visible

[Jan Glauber]

of_platform_device_destroy is the counterpart to
of_platform_device_create which is a non-static function.

After creating a platform device it might be necessary
to destroy it to deal with -EPROBE_DEFER where a
repeated of_platform_device_create call would fail otherwise.

Therefore also make of_platform_device_destroy globally visible.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/of/platform.c       | 3 ++-
 include/linux/of_platform.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 71fecc2..703a421 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -523,7 +523,7 @@ static int __init of_platform_default_populate_init(void)
 arch_initcall_sync(of_platform_default_populate_init);
 #endif
 
-static int of_platform_device_destroy(struct device *dev, void *data)
+int of_platform_device_destroy(struct device *dev, void *data)
 {
 	/* Do not touch devices not populated from the device tree */
 	if (!dev->of_node || !of_node_check_flag(dev->of_node, OF_POPULATED))
@@ -544,6 +544,7 @@ static int of_platform_device_destroy(struct device *dev, void *data)
 	of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(of_platform_device_destroy);
 
 /**
  * of_platform_depopulate() - Remove devices populated from device tree
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index dc8224a..e0d1946 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -64,6 +64,7 @@ extern struct platform_device *of_platform_device_create(struct device_node *np,
 						   const char *bus_id,
 						   struct device *parent);
 
+extern int of_platform_device_destroy(struct device *dev, void *data);
 extern int of_platform_bus_probe(struct device_node *root,
 				 const struct of_device_id *matches,
 				 struct device *parent);
-- 
2.9.0.rc0.21.g7777322

[PATCH 5/5] mmc: cavium: Fix probing race with regulator

[Jan Glauber]

If the regulator probing is not yet finished this driver
might catch a -EPROBE_DEFER. Returning after this condition
did not remove the created platform device. On a repeated
call to the probe function the of_platform_device_create
fails.

Calling of_platform_device_destroy after EPROBE_DEFER resolves
this bug.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/mmc/host/cavium-thunderx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
index fe3d772..257535e 100644
--- a/drivers/mmc/host/cavium-thunderx.c
+++ b/drivers/mmc/host/cavium-thunderx.c
@@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
 				continue;
 
 			ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
-			if (ret)
+			if (ret) {
+				of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);
 				goto error;
+			}
 		}
 		i++;
 	}
-- 
2.9.0.rc0.21.g7777322

Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator

[Rob Herring]

On Tue, May 16, 2017 at 4:36 AM, Jan Glauber <jglauber@cavium.com> wrote:
> If the regulator probing is not yet finished this driver
> might catch a -EPROBE_DEFER. Returning after this condition
> did not remove the created platform device. On a repeated
> call to the probe function the of_platform_device_create
> fails.
>
> Calling of_platform_device_destroy after EPROBE_DEFER resolves
> this bug.
>
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> ---
>  drivers/mmc/host/cavium-thunderx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
> index fe3d772..257535e 100644
> --- a/drivers/mmc/host/cavium-thunderx.c
> +++ b/drivers/mmc/host/cavium-thunderx.c
> @@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
>                                 continue;
>
>                         ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
> -                       if (ret)
> +                       if (ret) {
> +                               of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);

What if this fails after the 1st iteration of the loop. It's only
cleaning up the current device.

Use devm_of_platform_populate or
of_platform_populate/of_platform_depopulate instead.

Rob

Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator

[Jan Glauber]

On Tue, May 16, 2017 at 08:07:50AM -0500, Rob Herring wrote:
> On Tue, May 16, 2017 at 4:36 AM, Jan Glauber <jglauber@cavium.com> wrote:
> > If the regulator probing is not yet finished this driver
> > might catch a -EPROBE_DEFER. Returning after this condition
> > did not remove the created platform device. On a repeated
> > call to the probe function the of_platform_device_create
> > fails.
> >
> > Calling of_platform_device_destroy after EPROBE_DEFER resolves
> > this bug.
> >
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> >  drivers/mmc/host/cavium-thunderx.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
> > index fe3d772..257535e 100644
> > --- a/drivers/mmc/host/cavium-thunderx.c
> > +++ b/drivers/mmc/host/cavium-thunderx.c
> > @@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
> >                                 continue;
> >
> >                         ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
> > -                       if (ret)
> > +                       if (ret) {
> > +                               of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);
> 
> What if this fails after the 1st iteration of the loop. It's only
> cleaning up the current device.

The platform device is just a dummy device created directly before
cvm_mmc_of_slot_probe(). So there is no need to cleanup anything else.

As far as I've seen it the platform code 'tags' the nodes it already
used, but I need the same node to be parsed again on -EPROBE_DEFER.

> Use devm_of_platform_populate or
> of_platform_populate/of_platform_depopulate instead.

I'm not sure one of these will work here.

--Jan

> Rob

Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator

[Rob Herring]

On Tue, May 16, 2017 at 8:38 AM, Jan Glauber
<jan.glauber@caviumnetworks.com> wrote:
> On Tue, May 16, 2017 at 08:07:50AM -0500, Rob Herring wrote:
>> On Tue, May 16, 2017 at 4:36 AM, Jan Glauber <jglauber@cavium.com> wrote:
>> > If the regulator probing is not yet finished this driver
>> > might catch a -EPROBE_DEFER. Returning after this condition
>> > did not remove the created platform device. On a repeated
>> > call to the probe function the of_platform_device_create
>> > fails.
>> >
>> > Calling of_platform_device_destroy after EPROBE_DEFER resolves
>> > this bug.
>> >
>> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
>> > ---
>> >  drivers/mmc/host/cavium-thunderx.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
>> > index fe3d772..257535e 100644
>> > --- a/drivers/mmc/host/cavium-thunderx.c
>> > +++ b/drivers/mmc/host/cavium-thunderx.c
>> > @@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
>> >                                 continue;
>> >
>> >                         ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
>> > -                       if (ret)
>> > +                       if (ret) {
>> > +                               of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);
>>
>> What if this fails after the 1st iteration of the loop. It's only
>> cleaning up the current device.
>
> The platform device is just a dummy device created directly before
> cvm_mmc_of_slot_probe(). So there is no need to cleanup anything else.

So if you have 2 slots, the first slot probes successfully and the 2nd
slot defers, then you only need to clean-up the 2nd device/slot? Looks
to me like you are leaking the 1st device you alloc.

>
> As far as I've seen it the platform code 'tags' the nodes it already
> used, but I need the same node to be parsed again on -EPROBE_DEFER.
>
>> Use devm_of_platform_populate or
>> of_platform_populate/of_platform_depopulate instead.
>
> I'm not sure one of these will work here.

Those functions loop over child nodes and create devices. You are
doing the same thing. You'd just need to create all the devices first
and then probe them all.

The whole structure here with the dummy devices and how you are
initializing things is screwy. IMO, you should be creating actual
drivers for the dummy devices.

Rob

Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator

[David Daney]

On 05/16/2017 07:37 AM, Rob Herring wrote:
> On Tue, May 16, 2017 at 8:38 AM, Jan Glauber
> <jan.glauber@caviumnetworks.com> wrote:
>> On Tue, May 16, 2017 at 08:07:50AM -0500, Rob Herring wrote:
>>> On Tue, May 16, 2017 at 4:36 AM, Jan Glauber <jglauber@cavium.com> wrote:
>>>> If the regulator probing is not yet finished this driver
>>>> might catch a -EPROBE_DEFER. Returning after this condition
>>>> did not remove the created platform device. On a repeated
>>>> call to the probe function the of_platform_device_create
>>>> fails.
>>>>
>>>> Calling of_platform_device_destroy after EPROBE_DEFER resolves
>>>> this bug.
>>>>
>>>> Signed-off-by: Jan Glauber <jglauber@cavium.com>
>>>> ---
>>>>   drivers/mmc/host/cavium-thunderx.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
>>>> index fe3d772..257535e 100644
>>>> --- a/drivers/mmc/host/cavium-thunderx.c
>>>> +++ b/drivers/mmc/host/cavium-thunderx.c
>>>> @@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
>>>>                                  continue;
>>>>
>>>>                          ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
>>>> -                       if (ret)
>>>> +                       if (ret) {
>>>> +                               of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);
>>>
>>> What if this fails after the 1st iteration of the loop. It's only
>>> cleaning up the current device.
>>
>> The platform device is just a dummy device created directly before
>> cvm_mmc_of_slot_probe(). So there is no need to cleanup anything else.
> 
> So if you have 2 slots, the first slot probes successfully and the 2nd
> slot defers, then you only need to clean-up the 2nd device/slot? Looks
> to me like you are leaking the 1st device you alloc.
> 
>>
>> As far as I've seen it the platform code 'tags' the nodes it already
>> used, but I need the same node to be parsed again on -EPROBE_DEFER.
>>
>>> Use devm_of_platform_populate or
>>> of_platform_populate/of_platform_depopulate instead.
>>
>> I'm not sure one of these will work here.
> 
> Those functions loop over child nodes and create devices. You are
> doing the same thing. You'd just need to create all the devices first
> and then probe them all.
> 
> The whole structure here with the dummy devices and how you are
> initializing things is screwy. IMO, you should be creating actual
> drivers for the dummy devices.
> 

The "dummy devices" (AKA slots) share many resources with a need for 
locking between the slots to make sure these shared resources don't get 
stomped on by the other slots.  We did have discussions about the 
architecture of the driver around this very point in the review process 
before the current driver was merged.  The conclusion we reached was 
that we wouldn't fragment the code across multiple drivers.

I would suggest the following:

1) Patches 1-3 are independent of this discussion, and could probably be 
merged separately from 4 and 5.

2) Without creating invasive infrastructure changes in the MMC core, the 
current driver architecture is good enough, and we should patch it so 
there are no resource leaks in the cleanest manner possible.

Thanks,
David Daney

Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator

[Jan Glauber]

On Tue, May 16, 2017 at 09:37:48AM -0500, Rob Herring wrote:
> On Tue, May 16, 2017 at 8:38 AM, Jan Glauber
> <jan.glauber@caviumnetworks.com> wrote:
> > On Tue, May 16, 2017 at 08:07:50AM -0500, Rob Herring wrote:
> >> On Tue, May 16, 2017 at 4:36 AM, Jan Glauber <jglauber@cavium.com> wrote:
> >> > If the regulator probing is not yet finished this driver
> >> > might catch a -EPROBE_DEFER. Returning after this condition
> >> > did not remove the created platform device. On a repeated
> >> > call to the probe function the of_platform_device_create
> >> > fails.
> >> >
> >> > Calling of_platform_device_destroy after EPROBE_DEFER resolves
> >> > this bug.
> >> >
> >> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> >> > ---
> >> >  drivers/mmc/host/cavium-thunderx.c | 4 +++-
> >> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
> >> > index fe3d772..257535e 100644
> >> > --- a/drivers/mmc/host/cavium-thunderx.c
> >> > +++ b/drivers/mmc/host/cavium-thunderx.c
> >> > @@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
> >> >                                 continue;
> >> >
> >> >                         ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
> >> > -                       if (ret)
> >> > +                       if (ret) {
> >> > +                               of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);
> >>
> >> What if this fails after the 1st iteration of the loop. It's only
> >> cleaning up the current device.
> >
> > The platform device is just a dummy device created directly before
> > cvm_mmc_of_slot_probe(). So there is no need to cleanup anything else.
> 
> So if you have 2 slots, the first slot probes successfully and the 2nd
> slot defers, then you only need to clean-up the 2nd device/slot? Looks
> to me like you are leaking the 1st device you alloc.

OK, got it now. My assumption was that your scenario can't happen in
reality with EPROBE_DEFER.

> >
> > As far as I've seen it the platform code 'tags' the nodes it already
> > used, but I need the same node to be parsed again on -EPROBE_DEFER.
> >
> >> Use devm_of_platform_populate or
> >> of_platform_populate/of_platform_depopulate instead.
> >
> > I'm not sure one of these will work here.
> 
> Those functions loop over child nodes and create devices. You are
> doing the same thing. You'd just need to create all the devices first
> and then probe them all.

I'll take a look at devm_of_platform_populate then. If I can use it it
will solve the leak issue.

thanks,
Jan

> The whole structure here with the dummy devices and how you are
> initializing things is screwy. IMO, you should be creating actual
> drivers for the dummy devices.
> 
> Rob

Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator

[Jan Glauber]

On Wed, May 17, 2017 at 03:41:12PM +0200, Jan Glauber wrote:
> On Tue, May 16, 2017 at 09:37:48AM -0500, Rob Herring wrote:
> > On Tue, May 16, 2017 at 8:38 AM, Jan Glauber
> > <jan.glauber@caviumnetworks.com> wrote:
> > > On Tue, May 16, 2017 at 08:07:50AM -0500, Rob Herring wrote:
> > >> On Tue, May 16, 2017 at 4:36 AM, Jan Glauber <jglauber@cavium.com> wrote:
> > >> > If the regulator probing is not yet finished this driver
> > >> > might catch a -EPROBE_DEFER. Returning after this condition
> > >> > did not remove the created platform device. On a repeated
> > >> > call to the probe function the of_platform_device_create
> > >> > fails.
> > >> >
> > >> > Calling of_platform_device_destroy after EPROBE_DEFER resolves
> > >> > this bug.
> > >> >
> > >> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > >> > ---
> > >> >  drivers/mmc/host/cavium-thunderx.c | 4 +++-
> > >> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
> > >> > index fe3d772..257535e 100644
> > >> > --- a/drivers/mmc/host/cavium-thunderx.c
> > >> > +++ b/drivers/mmc/host/cavium-thunderx.c
> > >> > @@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
> > >> >                                 continue;
> > >> >
> > >> >                         ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
> > >> > -                       if (ret)
> > >> > +                       if (ret) {
> > >> > +                               of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);
> > >>
> > >> What if this fails after the 1st iteration of the loop. It's only
> > >> cleaning up the current device.
> > >
> > > The platform device is just a dummy device created directly before
> > > cvm_mmc_of_slot_probe(). So there is no need to cleanup anything else.
> > 
> > So if you have 2 slots, the first slot probes successfully and the 2nd
> > slot defers, then you only need to clean-up the 2nd device/slot? Looks
> > to me like you are leaking the 1st device you alloc.
> 
> OK, got it now. My assumption was that your scenario can't happen in
> reality with EPROBE_DEFER.
> 
> > >
> > > As far as I've seen it the platform code 'tags' the nodes it already
> > > used, but I need the same node to be parsed again on -EPROBE_DEFER.
> > >
> > >> Use devm_of_platform_populate or
> > >> of_platform_populate/of_platform_depopulate instead.
> > >
> > > I'm not sure one of these will work here.
> > 
> > Those functions loop over child nodes and create devices. You are
> > doing the same thing. You'd just need to create all the devices first
> > and then probe them all.
> 
> I'll take a look at devm_of_platform_populate then. If I can use it it
> will solve the leak issue.

Using [devm_]of_platform_populate/of_platform_depopulate would require
a platform driver with its prove/remove functions. I don't think this
would make the driver easier to read as we would then have a platform driver
within a pci driver.

If it is possible to export the of_platform_device_destroy() I would
prefer to fix the leak and stay with of_platform_device_create.

--Jan

> thanks,
> Jan
> 
> > The whole structure here with the dummy devices and how you are
> > initializing things is screwy. IMO, you should be creating actual
> > drivers for the dummy devices.
> > 
> > Rob

Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator

[Rob Herring]

On Thu, May 18, 2017 at 4:39 AM, Jan Glauber
<jan.glauber@caviumnetworks.com> wrote:
> On Wed, May 17, 2017 at 03:41:12PM +0200, Jan Glauber wrote:
>> On Tue, May 16, 2017 at 09:37:48AM -0500, Rob Herring wrote:
>> > On Tue, May 16, 2017 at 8:38 AM, Jan Glauber
>> > <jan.glauber@caviumnetworks.com> wrote:
>> > > On Tue, May 16, 2017 at 08:07:50AM -0500, Rob Herring wrote:
>> > >> On Tue, May 16, 2017 at 4:36 AM, Jan Glauber <jglauber@cavium.com> wrote:
>> > >> > If the regulator probing is not yet finished this driver
>> > >> > might catch a -EPROBE_DEFER. Returning after this condition
>> > >> > did not remove the created platform device. On a repeated
>> > >> > call to the probe function the of_platform_device_create
>> > >> > fails.
>> > >> >
>> > >> > Calling of_platform_device_destroy after EPROBE_DEFER resolves
>> > >> > this bug.
>> > >> >
>> > >> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
>> > >> > ---
>> > >> >  drivers/mmc/host/cavium-thunderx.c | 4 +++-
>> > >> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > >> >
>> > >> > diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
>> > >> > index fe3d772..257535e 100644
>> > >> > --- a/drivers/mmc/host/cavium-thunderx.c
>> > >> > +++ b/drivers/mmc/host/cavium-thunderx.c
>> > >> > @@ -137,8 +137,10 @@ static int thunder_mmc_probe(struct pci_dev *pdev,
>> > >> >                                 continue;
>> > >> >
>> > >> >                         ret = cvm_mmc_of_slot_probe(&host->slot_pdev[i]->dev, host);
>> > >> > -                       if (ret)
>> > >> > +                       if (ret) {
>> > >> > +                               of_platform_device_destroy(&host->slot_pdev[i]->dev, NULL);
>> > >>
>> > >> What if this fails after the 1st iteration of the loop. It's only
>> > >> cleaning up the current device.
>> > >
>> > > The platform device is just a dummy device created directly before
>> > > cvm_mmc_of_slot_probe(). So there is no need to cleanup anything else.
>> >
>> > So if you have 2 slots, the first slot probes successfully and the 2nd
>> > slot defers, then you only need to clean-up the 2nd device/slot? Looks
>> > to me like you are leaking the 1st device you alloc.
>>
>> OK, got it now. My assumption was that your scenario can't happen in
>> reality with EPROBE_DEFER.
>>
>> > >
>> > > As far as I've seen it the platform code 'tags' the nodes it already
>> > > used, but I need the same node to be parsed again on -EPROBE_DEFER.
>> > >
>> > >> Use devm_of_platform_populate or
>> > >> of_platform_populate/of_platform_depopulate instead.
>> > >
>> > > I'm not sure one of these will work here.
>> >
>> > Those functions loop over child nodes and create devices. You are
>> > doing the same thing. You'd just need to create all the devices first
>> > and then probe them all.
>>
>> I'll take a look at devm_of_platform_populate then. If I can use it it
>> will solve the leak issue.
>
> Using [devm_]of_platform_populate/of_platform_depopulate would require
> a platform driver with its prove/remove functions. I don't think this
> would make the driver easier to read as we would then have a platform driver
> within a pci driver.

How is that? of_platform_populate ultimately just calls
of_platform_device_create.

> If it is possible to export the of_platform_device_destroy() I would
> prefer to fix the leak and stay with of_platform_device_create.

In any case, we can just export it.

Rob

Re: [PATCH 0/5] mmc: cavium: bug fixes for 4.12

[Ulf Hansson]

On 16 May 2017 at 11:36, Jan Glauber <jglauber@cavium.com> wrote:
> Hi Ulf,
>
> here are some bug fixes for the new mmc driver. The only
> non-trivial fix should be the platform thing in patch #4 and #5.
>
> Tested on OcteonTx and on various MIPS boxes.
>
> thanks,
> Jan
>
> David Daney (2):
>   mmc: cavium-octeon: Fix interrupt enable code
>   mmc: cavium-octeon: Use proper GPIO name for power control
>
> Jan Glauber (3):
>   mmc: cavium: Prevent crash with incomplete DT
>   of/platform: Make of_platform_device_destroy globally visible
>   mmc: cavium: Fix probing race with regulator
>
>  drivers/mmc/host/cavium-octeon.c   | 4 ++--
>  drivers/mmc/host/cavium-thunderx.c | 4 +++-
>  drivers/mmc/host/cavium.c          | 8 ++++++--
>  drivers/of/platform.c              | 3 ++-
>  include/linux/of_platform.h        | 1 +
>  5 files changed, 14 insertions(+), 6 deletions(-)
>
> --
> 2.9.0.rc0.21.g7777322
>

I picked up patch1 and patch2 for fixes. The rest needs a re-spin.

Thanks and kind regards
Uffe

Re: [PATCH 0/5] mmc: cavium: bug fixes for 4.12

[Jan Glauber]

On Fri, May 19, 2017 at 10:30:22AM +0200, Ulf Hansson wrote:
> On 16 May 2017 at 11:36, Jan Glauber <jglauber@cavium.com> wrote:
> > Hi Ulf,
> >
> > here are some bug fixes for the new mmc driver. The only
> > non-trivial fix should be the platform thing in patch #4 and #5.
> >
> > Tested on OcteonTx and on various MIPS boxes.
> >
> > thanks,
> > Jan
> >
> > David Daney (2):
> >   mmc: cavium-octeon: Fix interrupt enable code
> >   mmc: cavium-octeon: Use proper GPIO name for power control
> >
> > Jan Glauber (3):
> >   mmc: cavium: Prevent crash with incomplete DT
> >   of/platform: Make of_platform_device_destroy globally visible
> >   mmc: cavium: Fix probing race with regulator
> >
> >  drivers/mmc/host/cavium-octeon.c   | 4 ++--
> >  drivers/mmc/host/cavium-thunderx.c | 4 +++-
> >  drivers/mmc/host/cavium.c          | 8 ++++++--
> >  drivers/of/platform.c              | 3 ++-
> >  include/linux/of_platform.h        | 1 +
> >  5 files changed, 14 insertions(+), 6 deletions(-)
> >
> > --
> > 2.9.0.rc0.21.g7777322
> >
> 
> I picked up patch1 and patch2 for fixes. The rest needs a re-spin.

I'll re-spin the other patches.

thanks,
Jan


> Thanks and kind regards
> Uffe