MMC: fix sdhci-dove removal
PATCH 1/1 MMC: fix sdhci-dove removal
1. Unregister the device _BEFORE_ taking away any resources it may be using.
2. Don't check clks against NULL.
---
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(-)
drivers/mmc/host/sdhci-dove.c +6 -7
@@ -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 = {
PATCH 1/1 MMC: fix sdhci-dove removal
Quoted 5 lines · Russell King - ARM Linux
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 <[email protected]>
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.
PATCH 1/1 MMC: fix sdhci-dove removal
Quoted 31 lines · Russell King - ARM Linux
Quoted 5 lines · Russell King - ARM Linux
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 <[email protected]>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_iosSee, 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.
Quoted 2 lines
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 <[email protected]>
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.
--- drivers/mmc/host/sdhci-dove.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-)
drivers/mmc/host/sdhci-dove.c +18 -18
@@ -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 = {
Signature · 1 line
1.7.4.4
PATCH 1/1 MMC: fix sdhci-dove removal
Quoted 66 lines · Russell King - ARM Linux
Quoted 31 lines · Russell King - ARM Linux
Quoted 5 lines · Russell King - ARM Linux
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 <[email protected]>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_iosSee, 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.
Quoted 2 lines
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 <[email protected]>
Subject: [PATCH] MMC: fix sdhci-dove probe/removal1. 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 <[email protected]>--- drivers/mmc/host/sdhci-dove.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-)drivers/mmc/host/sdhci-dove.c +0
-4@@ -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; -
drivers/mmc/host/sdhci-dove.c static int __devinit sdhci_dove_probe(struct platform_device *pdev) · line 93
Quoted 10 lines
drivers/mmc/host/sdhci-dove.c +2
-2priv =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.
PATCH 1/1 MMC: fix sdhci-dove removal
Quoted 76 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 75 lines · Russell King - ARM Linux
Quoted 31 lines · Russell King - ARM Linux
Quoted 5 lines · Russell King - ARM Linux
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 <[email protected]>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_iosSee, 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.
Quoted 2 lines
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 <[email protected]>
Subject: [PATCH] MMC: fix sdhci-dove probe/removal1. 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 <[email protected]>--- drivers/mmc/host/sdhci-dove.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-)drivers/mmc/host/sdhci-dove.c +1
-6@@ -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 ; }
Quoted 4 lines
drivers/mmc/host/sdhci-dove.c static int __devinit sdhci_dove_probe(struct platform_device *pdev) · line 93
Quoted 1 line
drivers/mmc/host/sdhci-dove.c +1
-0+ priv->clk =clk_get (&pdev->dev,NULL );you have
devm_clk_gettoomaybe 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.
PATCH 1/1 MMC: fix sdhci-dove removal
In reply to Russell King - ARM LinuxHi Russell,
Quoted 8 lines · Russell King - ARM Linux
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 <[email protected]>
Subject: [PATCH] MMC: fix sdhci-dove probe/removal
Quoted 7 lines
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 <[email protected]>
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.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
PATCH 1/1 MMC: fix sdhci-dove removal
Quoted 17 lines · Chris Ball
Hi Russell,
Quoted 14 lines · Russell King - ARM Linux
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 <[email protected]>
Subject: [PATCH] MMC: fix sdhci-dove probe/removal1. 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.
Quoted 4 lines
Quoted 1 line
Signed-off-by Russell King <[email protected]>This version of the patch doesn't apply cleanly or compile -- maybe you have a later revision?
Well, I'm only going to send a patch against 3.6 because that's the kernel I'm running on the cubox - which is the platform I'm finding these bugs on. As it's a platform which I want to keep stable for other work, it's not getting -rc kernels on it.
It's also been through several revisions across conflict resolutions which is why the above errors occurred (thanks to having to fix them up multiple times.) And to top it off, there's additional patches below this one too, so separating out just the fix is virtually impossible to do safely.
Anyway, here's a replacement patch, updated against my cubox tree but lacking the cubox gpio changes for it. Un-tested through: (a) I can't test this on its own on the cubox, (b) I don't have a separate dove tree to test it against and my disk space is all but out at the moment for yet another build tree...
It would helpful of course to have _more_ of the cubox stuff in mainline but unfortunately it's all non-DT so wouldn't be accepted.
drivers/mmc/host/sdhci-dove.c | 38 ++++++++++++++++++++------------------ 1 files changed, 20 insertions(+), 18 deletions(-)
drivers/mmc/host/sdhci-dove.c +20 -18
@@ -19,6 +19,7 @@* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */ +#include <linux/err.h> #include <linux/io.h> #include <linux/clk.h> #include <linux/err.h>
@@ -83,30 +84,32 @@ 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: +if (!IS_ERR (priv->clk)) { +clk_disable_unprepare (priv->clk); +clk_put (priv->clk); + }return ret; }
@@ -116,14 +119,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 = {
PATCH 1/1 MMC: fix sdhci-dove removal
Hi,
Quoted 5 lines · Russell King - ARM Linux
Anyway, here's a replacement patch, updated against my cubox tree but lacking the cubox gpio changes for it. Un-tested through: (a) I can't test this on its own on the cubox, (b) I don't have a separate dove tree to test it against and my disk space is all but out at the moment for yet another build tree...
Compile-tested and pushed to mmc-next, and I'll submit it for 3.7 if I get a Tested-by from someone like Sebastian (CC'd), or for 3.8 if I don't. (I'll add sdhci-pltfm API rework to my TODO list, too.)
Thanks,
- Chris.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
PATCH 1/1 MMC: fix sdhci-dove removal
Quoted 10 lines · Chris Ball
Quoted 5 lines · Russell King - ARM Linux
Anyway, here's a replacement patch, updated against my cubox tree but lacking the cubox gpio changes for it. Un-tested through: (a) I can't test this on its own on the cubox, (b) I don't have a separate dove tree to test it against and my disk space is all but out at the moment for yet another build tree...
Compile-tested and pushed to mmc-next, and I'll submit it for 3.7 if I get a Tested-by from someone like Sebastian (CC'd), or for 3.8 if I don't. (I'll add sdhci-pltfm API rework to my TODO list, too.)
Chris, Russell,
obviously I am not the one to ask for a Tested-by because it was my patch that originally caused all the trouble. My apologies for this!
Anyway, I did test the patch on 3.7-rc3 and except that you need a 3-way git am because of a one-line offset, sdhci-dove runs just fine.
PATCH 1/1 MMC: fix sdhci-dove removal
Hi,
Quoted 14 lines · Sebastian Hesselbarth
Quoted 10 lines · Chris Ball
Quoted 5 lines · Russell King - ARM Linux
Anyway, here's a replacement patch, updated against my cubox tree but lacking the cubox gpio changes for it. Un-tested through: (a) I can't test this on its own on the cubox, (b) I don't have a separate dove tree to test it against and my disk space is all but out at the moment for yet another build tree...
Compile-tested and pushed to mmc-next, and I'll submit it for 3.7 if I get a Tested-by from someone like Sebastian (CC'd), or for 3.8 if I don't. (I'll add sdhci-pltfm API rework to my TODO list, too.)
Chris, Russell,
Quoted 7 lines
obviously I am not the one to ask for a Tested-by because it was my patch that originally caused all the trouble. My apologies for this!
Anyway, I did test the patch on 3.7-rc3 and except that you need a 3-way git am because of a one-line offset, sdhci-dove runs just fine.
Great, thanks -- will push to 3.7.
- Chris.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child