| Message ID | 20121015104456.GR21164@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11:44 Mon 15 Oct , Russell King - ARM Linux wrote: > On Mon, Oct 15, 2012 at 11:37:25AM +0100, Russell King - ARM Linux wrote: > > On Mon, Oct 15, 2012 at 10:43:48AM +0100, Russell King - ARM Linux wrote: > > > 1. Unregister the device _BEFORE_ taking away any resources it may > > > be using. > > > 2. Don't check clks against NULL. > > > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > > > Looking at this driver some more, who the hell came up with the sdhci > > registration interface? It violates one of the most fundamental > > principles of kernel driver programming. You do _NOT_ publish your > > driver interfaces _UNTIL_ you have finished setting your device up. > > Otherwise, in a preemptible or SMP kernel, your driver can be used > > before the initialization has completed. > > > > As this driver calls sdhci_pltfm_register() before it has obtained the > > clock for the interface, and this function does: > > sdhci_pltfm_init > > sdhci_add_host > > mmc_add_host > > mmc_start_host > > mmc_power_up > > mmc_set_ios > > sdhci_set_ios > > > > See, we're trying to power up and clock the card _before_ the dove > > sdhci driver has even claimed the clock let alone enabled it. This > > is total bollocks. The sdhci platform interface is total crap for > > creating this broken design in the first place. This is why MMC has > > the init + add interfaces, they're there to allow drivers to do stuff > > the Right(tm) way and avoid shit like the above. > > > > This should have been picked up at review time before the driver went > > into mainline. In any case, it needs to be fixed. > > Here's an updated patch which just about fixes the sdhci-dove driver. > I would not be surprised given the idiotic sdhci-pltfm API if many > other drivers suffered the same bug. > > 8<==== > From: Russell King <rmk+kernel@arm.linux.org.uk> > Subject: [PATCH] MMC: fix sdhci-dove probe/removal > > 1. Never ever publish a device in the system before it has been setup > to a usable state. > 2. Unregister the device _BEFORE_ taking away any resources it may be > using. > 3. Don't check clks against NULL. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/mmc/host/sdhci-dove.c | 36 ++++++++++++++++++------------------ > 1 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c > index a6e53a1..7d3a4e4 100644 > --- a/drivers/mmc/host/sdhci-dove.c > +++ b/drivers/mmc/host/sdhci-dove.c > @@ -83,30 +83,31 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev) > struct sdhci_dove_priv *priv; > int ret; > > - ret = sdhci_pltfm_register(pdev, &sdhci_dove_pdata); > - if (ret) > - goto sdhci_dove_register_fail; > - > priv = devm_kzalloc(&pdev->dev, sizeof(struct sdhci_dove_priv), > GFP_KERNEL); > if (!priv) { > dev_err(&pdev->dev, "unable to allocate private data"); > - ret = -ENOMEM; > - goto sdhci_dove_allocate_fail; > + return -ENOMEM; > } > > + priv->clk = clk_get(&pdev->dev, NULL); you have devm_clk_get too maybe you could use it here too Best Regards, J. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 15, 2012 at 04:58:55PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 11:44 Mon 15 Oct , Russell King - ARM Linux wrote: > > On Mon, Oct 15, 2012 at 11:37:25AM +0100, Russell King - ARM Linux wrote: > > > On Mon, Oct 15, 2012 at 10:43:48AM +0100, Russell King - ARM Linux wrote: > > > > 1. Unregister the device _BEFORE_ taking away any resources it may > > > > be using. > > > > 2. Don't check clks against NULL. > > > > > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > > > > > Looking at this driver some more, who the hell came up with the sdhci > > > registration interface? It violates one of the most fundamental > > > principles of kernel driver programming. You do _NOT_ publish your > > > driver interfaces _UNTIL_ you have finished setting your device up. > > > Otherwise, in a preemptible or SMP kernel, your driver can be used > > > before the initialization has completed. > > > > > > As this driver calls sdhci_pltfm_register() before it has obtained the > > > clock for the interface, and this function does: > > > sdhci_pltfm_init > > > sdhci_add_host > > > mmc_add_host > > > mmc_start_host > > > mmc_power_up > > > mmc_set_ios > > > sdhci_set_ios > > > > > > See, we're trying to power up and clock the card _before_ the dove > > > sdhci driver has even claimed the clock let alone enabled it. This > > > is total bollocks. The sdhci platform interface is total crap for > > > creating this broken design in the first place. This is why MMC has > > > the init + add interfaces, they're there to allow drivers to do stuff > > > the Right(tm) way and avoid shit like the above. > > > > > > This should have been picked up at review time before the driver went > > > into mainline. In any case, it needs to be fixed. > > > > Here's an updated patch which just about fixes the sdhci-dove driver. > > I would not be surprised given the idiotic sdhci-pltfm API if many > > other drivers suffered the same bug. > > > > 8<==== > > From: Russell King <rmk+kernel@arm.linux.org.uk> > > Subject: [PATCH] MMC: fix sdhci-dove probe/removal > > > > 1. Never ever publish a device in the system before it has been setup > > to a usable state. > > 2. Unregister the device _BEFORE_ taking away any resources it may be > > using. > > 3. Don't check clks against NULL. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > > drivers/mmc/host/sdhci-dove.c | 36 ++++++++++++++++++------------------ > > 1 files changed, 18 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c > > index a6e53a1..7d3a4e4 100644 > > --- a/drivers/mmc/host/sdhci-dove.c > > +++ b/drivers/mmc/host/sdhci-dove.c > > @@ -83,30 +83,31 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev) > > struct sdhci_dove_priv *priv; > > int ret; > > > > - ret = sdhci_pltfm_register(pdev, &sdhci_dove_pdata); > > - if (ret) > > - goto sdhci_dove_register_fail; > > - > > priv = devm_kzalloc(&pdev->dev, sizeof(struct sdhci_dove_priv), > > GFP_KERNEL); > > if (!priv) { > > dev_err(&pdev->dev, "unable to allocate private data"); > > - ret = -ENOMEM; > > - goto sdhci_dove_allocate_fail; > > + return -ENOMEM; > > } > > > > + priv->clk = clk_get(&pdev->dev, NULL); > you have devm_clk_get too > > maybe you could use it here too This isn't a cleanup patch, this is a patch just to fix some stupid bugs in this code. If someone wants to convert that afterwards, it should be an entirely separate patch. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Russell, On Mon, Oct 15 2012, Russell King - ARM Linux wrote: > Here's an updated patch which just about fixes the sdhci-dove driver. > I would not be surprised given the idiotic sdhci-pltfm API if many > other drivers suffered the same bug. > > 8<==== > From: Russell King <rmk+kernel@arm.linux.org.uk> > Subject: [PATCH] MMC: fix sdhci-dove probe/removal > > 1. Never ever publish a device in the system before it has been setup > to a usable state. > 2. Unregister the device _BEFORE_ taking away any resources it may be > using. > 3. Don't check clks against NULL. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> This version of the patch doesn't apply cleanly or compile -- maybe you have a later revision? /home/cjb/git/mmc/drivers/mmc/host/sdhci-dove.c: In function ‘sdhci_dove_probe’: /home/cjb/git/mmc/drivers/mmc/host/sdhci-dove.c:109:2: error: implicit declaration of function ‘clk_unprepare_disable’ [-Werror=implicit-function-declaration] /home/cjb/git/mmc/drivers/mmc/host/sdhci-dove.c:111:1: warning: label ‘sdhci_dove_allocate_fail’ defined but not used [-Wunused-label] Thanks, - Chris.
diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c index a6e53a1..7d3a4e4 100644 --- a/drivers/mmc/host/sdhci-dove.c +++ b/drivers/mmc/host/sdhci-dove.c @@ -83,30 +83,31 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev) struct sdhci_dove_priv *priv; int ret; - ret = sdhci_pltfm_register(pdev, &sdhci_dove_pdata); - if (ret) - goto sdhci_dove_register_fail; - priv = devm_kzalloc(&pdev->dev, sizeof(struct sdhci_dove_priv), GFP_KERNEL); if (!priv) { dev_err(&pdev->dev, "unable to allocate private data"); - ret = -ENOMEM; - goto sdhci_dove_allocate_fail; + return -ENOMEM; } + priv->clk = clk_get(&pdev->dev, NULL); + if (!IS_ERR(priv->clk)) + clk_prepare_enable(priv->clk); + + ret = sdhci_pltfm_register(pdev, &sdhci_dove_pdata); + if (ret) + goto sdhci_dove_register_fail; + host = platform_get_drvdata(pdev); pltfm_host = sdhci_priv(host); pltfm_host->priv = priv; - priv->clk = clk_get(&pdev->dev, NULL); - if (!IS_ERR(priv->clk)) - clk_prepare_enable(priv->clk); return 0; -sdhci_dove_allocate_fail: - sdhci_pltfm_unregister(pdev); sdhci_dove_register_fail: + clk_unprepare_disable(priv->clk); + clk_put(priv->clk); +sdhci_dove_allocate_fail: return ret; } @@ -116,14 +117,13 @@ static int __devexit sdhci_dove_remove(struct platform_device *pdev) struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_dove_priv *priv = pltfm_host->priv; - if (priv->clk) { - if (!IS_ERR(priv->clk)) { - clk_disable_unprepare(priv->clk); - clk_put(priv->clk); - } - devm_kfree(&pdev->dev, priv->clk); + sdhci_pltfm_unregister(pdev); + + if (!IS_ERR(priv->clk)) { + clk_disable_unprepare(priv->clk); + clk_put(priv->clk); } - return sdhci_pltfm_unregister(pdev); + return 0; } static struct platform_driver sdhci_dove_driver = {