| 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.
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.
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 = {