Re: [PATCH] sdhci: put regulator if probe fails
On 10/13/2012 09:55 AM, Thadeu Lima de Souza Cascardo wrote:
When using the dummy regulator, SDHCI may fail its probing because the
regulator does not support any voltages.
When reloading the driver, you will get a warning about a duplicate
sysfs link.
[72211.963386] :03:00.0 supply vmmc not found, using dummy regulator
[72211.963409] [ cut here ]
[72211.963420] WARNING: at fs/sysfs/dir.c:536 sysfs_add_one+0x99/0xad()
[72211.963424] Hardware name:
[72211.963429] sysfs: cannot create duplicate filename
'/devices/platform/reg-dummy/regulator/regulator.0/:03:00.0-vmmc'
Avoid this by properly cleaning up when the probe fails calling
regulator_put. Other fail paths get fixed as well.
Signed-off-by: Thadeu Lima de Souza Cascardo casca...@holoscopio.com
---
drivers/mmc/host/sdhci.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7922adb..dd6bc26 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2992,7 +2992,8 @@ int sdhci_add_host(struct sdhci_host *host)
if (mmc-ocr_avail == 0) {
pr_err(%s: Hardware doesn't report any
support voltages.\n, mmc_hostname(mmc));
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_vmmc;
}
spin_lock_init(host-lock);
@@ -3121,6 +3122,9 @@ reset:
untasklet:
tasklet_kill(host-card_tasklet);
tasklet_kill(host-finish_tasklet);
+out_vmmc:
+ if (host-vmmc)
+ regulator_put(host-vmmc);
If fall down to untasklet, need not to disable regulator for vmmc?
Best Regards,
Jaehoon Chung
return ret;
}
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node
Hi Grant,
On 13.10.2012 02:05, Grant Likely wrote:
Balaji T K balaj...@ti.com wrote:
On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:
On 12.10.2012 16:56, Balaji T K wrote:
On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
On 12.10.2012 12:58, Daniel Mack wrote:
diff --git a/drivers/mmc/host/omap_hsmmc.c
b/drivers/mmc/host/omap_hsmmc.c
index 19ccb59..4b70823 100644 ---
a/drivers/mmc/host/omap_hsmmc.c +++
b/drivers/mmc/host/omap_hsmmc.c @@ -1728,6 +1728,7 @@
static int __devinit omap_hsmmc_probe(struct
platform_device *pdev)
const u16 *offsetp = match-data; pdata-reg_offset =
*offsetp; } +pdev-dev.platform_data = pdata; }
if (pdata == NULL) {
FWIW, this is the Oops I see without this patch:
Hi, Shouldn't pdev-dev.platform_data be set to NULL on _remove
?
Why?
To make sure on second insmod it is NULL, When built as module, So
that of_get_hsmmc_pdata is called to create pdata.
Actually the driver should *never* modify the value of
dev-platform_data. Ever.
That's interesting, because many drivers do this, especially since they
were converted to DT probing. Mostly because that way, the platform data
logic in callback functions can be reused, and often the platform
specific data is only stored in pdata and taken from there during the
lifetime of a device.
Is there any particular reason why this approach is frowned upon?
Make a copy instead.
A copy of what exactly? Of all members of the legacy pdata you mean?
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node
On Sat, Oct 13, 2012 at 9:05 AM, Daniel Mack zon...@gmail.com wrote:
Hi Grant,
On 13.10.2012 02:05, Grant Likely wrote:
Balaji T K balaj...@ti.com wrote:
On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:
On 12.10.2012 16:56, Balaji T K wrote:
On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
On 12.10.2012 12:58, Daniel Mack wrote:
diff --git a/drivers/mmc/host/omap_hsmmc.c
b/drivers/mmc/host/omap_hsmmc.c
index 19ccb59..4b70823 100644 ---
a/drivers/mmc/host/omap_hsmmc.c +++
b/drivers/mmc/host/omap_hsmmc.c @@ -1728,6 +1728,7 @@
static int __devinit omap_hsmmc_probe(struct
platform_device *pdev)
const u16 *offsetp = match-data; pdata-reg_offset =
*offsetp; } +pdev-dev.platform_data = pdata; }
if (pdata == NULL) {
FWIW, this is the Oops I see without this patch:
Hi, Shouldn't pdev-dev.platform_data be set to NULL on _remove
?
Why?
To make sure on second insmod it is NULL, When built as module, So
that of_get_hsmmc_pdata is called to create pdata.
Actually the driver should *never* modify the value of
dev-platform_data. Ever.
That's interesting, because many drivers do this, especially since they
were converted to DT probing. Mostly because that way, the platform data
logic in callback functions can be reused, and often the platform
specific data is only stored in pdata and taken from there during the
lifetime of a device.
Is there any particular reason why this approach is frowned upon?
Yes. The platform data pointer is owned by the code that registered
the platform device, not by the device driver. Some drivers do it, but
it is definitely illegal. I should add code to the platform bus core
code to throw a warning to any drivers that do that. It is a problem
because it becomes easy to mess up the lifetime model of device data,
particularly when it comes to unbinding/rebinding devices. For
example, if a driver changes the pdata pointer and then gets unbound,
then there will be a stale pdata pointer that may point to either
incorrect data or a data structure that is no longer allocated. You
could argue that it is fine if the driver is smart about how it cleans
up after itself, but in my experience driver authors rarely get it
correct and it results in a lot more code than is necessary. It is far
better for the driver to either grab all the data it needs out of
pdata at .probe() time, or to keep a copy of the 'correct' pdata
(correct depending on where the device retrieved it's data) in it's
private data structure instead of modifying the device.
Make a copy instead.
A copy of what exactly? Of all members of the legacy pdata you mean?
Yes. If the driver directly accesses the pdata structure outside of
the .probe() hook, then it should be modified to either copy the pdata
into the driver's private data structure, or it should copy the
pointer to the pdata so that OF usage can allocate one itself. For
example:
somedriver_probe(struct platform_device *pdev)
{
struct somedriver_private *somedriver;
somedriver = devm_kzalloc(sizeof (*somedriver), GFP_KERNEL);
somedriver-pdata = pdev-platform_data;
if (OF)
somedriver-pdata = devm_kzalloc(sizeof
(*somedriver-pdata), GFP_KERNEL);
}
The bonus with using devm_kzalloc is the driver doesn't even need to
do anything special to undo these allocations on failure or release.
:-)
g.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node
On 13.10.2012 10:48, Grant Likely wrote:
On Sat, Oct 13, 2012 at 9:05 AM, Daniel Mack zon...@gmail.com wrote:
Hi Grant,
On 13.10.2012 02:05, Grant Likely wrote:
Balaji T K balaj...@ti.com wrote:
On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:
On 12.10.2012 16:56, Balaji T K wrote:
On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
On 12.10.2012 12:58, Daniel Mack wrote:
diff --git a/drivers/mmc/host/omap_hsmmc.c
b/drivers/mmc/host/omap_hsmmc.c
index 19ccb59..4b70823 100644 ---
a/drivers/mmc/host/omap_hsmmc.c +++
b/drivers/mmc/host/omap_hsmmc.c @@ -1728,6 +1728,7 @@
static int __devinit omap_hsmmc_probe(struct
platform_device *pdev)
const u16 *offsetp = match-data; pdata-reg_offset =
*offsetp; } +pdev-dev.platform_data = pdata; }
if (pdata == NULL) {
FWIW, this is the Oops I see without this patch:
Hi, Shouldn't pdev-dev.platform_data be set to NULL on _remove
?
Why?
To make sure on second insmod it is NULL, When built as module, So
that of_get_hsmmc_pdata is called to create pdata.
Actually the driver should *never* modify the value of
dev-platform_data. Ever.
That's interesting, because many drivers do this, especially since they
were converted to DT probing. Mostly because that way, the platform data
logic in callback functions can be reused, and often the platform
specific data is only stored in pdata and taken from there during the
lifetime of a device.
Is there any particular reason why this approach is frowned upon?
Yes. The platform data pointer is owned by the code that registered
the platform device, not by the device driver. Some drivers do it, but
it is definitely illegal. I should add code to the platform bus core
code to throw a warning to any drivers that do that. It is a problem
because it becomes easy to mess up the lifetime model of device data,
particularly when it comes to unbinding/rebinding devices. For
example, if a driver changes the pdata pointer and then gets unbound,
then there will be a stale pdata pointer that may point to either
incorrect data or a data structure that is no longer allocated. You
could argue that it is fine if the driver is smart about how it cleans
up after itself, but in my experience driver authors rarely get it
correct and it results in a lot more code than is necessary. It is far
better for the driver to either grab all the data it needs out of
pdata at .probe() time, or to keep a copy of the 'correct' pdata
(correct depending on where the device retrieved it's data) in it's
private data structure instead of modifying the device.
Ok, interesting. Thanks for writing this up.
Make a copy instead.
A copy of what exactly? Of all members of the legacy pdata you mean?
Yes. If the driver directly accesses the pdata structure outside of
the .probe() hook, then it should be modified to either copy the pdata
into the driver's private data structure, or it should copy the
pointer to the pdata so that OF usage can allocate one itself. For
example:
somedriver_probe(struct platform_device *pdev)
{
struct somedriver_private *somedriver;
somedriver = devm_kzalloc(sizeof (*somedriver), GFP_KERNEL);
somedriver-pdata = pdev-platform_data;
if (OF)
somedriver-pdata = devm_kzalloc(sizeof
(*somedriver-pdata), GFP_KERNEL);
}
The bonus with using devm_kzalloc is the driver doesn't even need to
do anything special to undo these allocations on failure or release.
:-)
Ok, understood. Will keep an eye on this in the future. Thanks again for
the explanation.
For this particular driver, this means that both my and Balaji's ways of
fixing this are wrong?
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node
Daniel Mack zon...@gmail.com wrote:
On 13.10.2012 10:48, Grant Likely wrote:
somedriver_probe(struct platform_device *pdev)
{
struct somedriver_private *somedriver;
somedriver = devm_kzalloc(sizeof (*somedriver), GFP_KERNEL);
somedriver-pdata = pdev-platform_data;
if (OF)
somedriver-pdata = devm_kzalloc(sizeof
(*somedriver-pdata), GFP_KERNEL);
}
The bonus with using devm_kzalloc is the driver doesn't even need to
do anything special to undo these allocations on failure or release.
:-)
Ok, understood. Will keep an eye on this in the future. Thanks again
for
the explanation.
For this particular driver, this means that both my and Balaji's ways
of
fixing this are wrong?
Balaji's patch looks fine since it uses a local copy of the platform data
structure. It appears that the driver is already creating a local copy and
Balaji is just bug fixing call sites that aren't using it yet.
g.
--
Grant Likely, P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
