Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642360 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] mmc: cavium: bug fixes for 4.12 Jan Glauber <jglauber@cavium.com> - 2017-05-16 11:40 +0200
    [PATCH 4/5] of/platform: Make of_platform_device_destroy globally visible Jan Glauber <jglauber@cavium.com> - 2017-05-16 11:40 +0200
    [PATCH 3/5] mmc: cavium: Prevent crash with incomplete DT Jan Glauber <jglauber@cavium.com> - 2017-05-16 11:40 +0200
      Re: [PATCH 3/5] mmc: cavium: Prevent crash with incomplete DT Ulf Hansson <ulf.hansson@linaro.org> - 2017-05-19 09:20 +0200
    [PATCH 2/5] mmc: cavium-octeon: Use proper GPIO name for power control Jan Glauber <jglauber@cavium.com> - 2017-05-16 11:40 +0200
    [PATCH 5/5] mmc: cavium: Fix probing race with regulator Jan Glauber <jglauber@cavium.com> - 2017-05-16 11:40 +0200
      Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator Rob Herring <robh+dt@kernel.org> - 2017-05-16 15:10 +0200
        Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator Rob Herring <robh+dt@kernel.org> - 2017-05-16 16:50 +0200
          Re: [PATCH 5/5] mmc: cavium: Fix probing race with regulator Rob Herring <robh+dt@kernel.org> - 2017-05-18 15:00 +0200
    Re: [PATCH 0/5] mmc: cavium: bug fixes for 4.12 Ulf Hansson <ulf.hansson@linaro.org> - 2017-05-19 10:40 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1645378

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web