diff mbox

MMC: fix sdhci-dove removal

Message ID 20121015094348.GP21164@n2100.arm.linux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King - ARM Linux Oct. 15, 2012, 9:43 a.m. UTC
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(-)

Comments

Russell King - ARM Linux Oct. 15, 2012, 10:37 a.m. UTC | #1
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 mbox

Patch

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