| Message ID | 20121015094348.GP21164@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c index 90140eb..c11db64 100644 --- a/drivers/mmc/host/sdhci-dove.c +++ b/drivers/mmc/host/sdhci-dove.c @@ -117,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 const struct of_device_id sdhci_dove_of_match_table[] __devinitdata = {
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> --- I noticed this while merging v3.6 into Rabeeh's cubox kernel. drivers/mmc/host/sdhci-dove.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-)