mmc: mvsdio: Replace IS_ERR_NULL() is with IS_ERR()
Subsystems (2)
PATCH 1/1 mmc: mvsdio: Replace IS_ERR_NULL() is with IS_ERR()
A NULL is a valid clk cookie, so we should not be tested with IS_ERR_NULL(). Replace it with IS_ERR().
--- drivers/mmc/host/mvsdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mmc/host/mvsdio.c +1 -1
@@ -839,7 +839,7 @@ out:if (r)release_resource (r);if (mmc) -if (!IS_ERR_OR_NULL (host->clk)) { +if (!IS_ERR (host->clk)) {clk_disable_unprepare (host->clk);clk_put (host->clk); }
Signature · 1 line
1.7.10.4
PATCH 1/1 mmc: mvsdio: Replace IS_ERR_NULL() is with IS_ERR()
Quoted 7 lines · Andrew Lunn
A NULL is a valid clk cookie, so we should not be tested with
IS_ERR_NULL(). Replace it withIS_ERR().Signed-off-by Andrew Lunn <[email protected]>--- drivers/mmc/host/mvsdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
The subject and commit entry should be IS_ERR_OR_NULL, but other than that:
thx,
Jason.
PATCH 1/1 mmc: mvsdio: Replace IS_ERR_NULL() is with IS_ERR()
In reply to Andrew LunnHi Andrew,
Quoted 13 lines · Andrew Lunn
A NULL is a valid clk cookie, so we should not be tested with
IS_ERR_NULL(). Replace it withIS_ERR().Signed-off-by Andrew Lunn <[email protected]>--- drivers/mmc/host/mvsdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/mmc/host/mvsdio.c +0
-0
drivers/mmc/host/mvsdio.c out: · line 845
Quoted 8 lines
drivers/mmc/host/mvsdio.c +1
-1@@ -839,7 +839,7 @@ out:if (r)release_resource (r);if (mmc) -if (!IS_ERR_OR_NULL (host->clk)) { +if (!IS_ERR (host->clk)) {clk_disable_unprepare (host->clk);clk_put (host->clk); }
Hmm. I think this check was originally intended to go under "if (host)" above and not here. Notice the indentation mismatch/missing braces for "if (mmc)". My bad.
Additionally, host->clk == NULL is not necessarily the result of clk_get() but it can also be NULL if the irq could not be assigned. Leading to unbalanced clk calls in this case.
- Simon
PATCH v2 1/1 mmc: mvsdio: Replace IS_ERR_OR_NULL() with IS_ERR()
A NULL is a valid clk cookie, so we should not be tested with IS_ERR_OR_NULL(). Replace it with IS_ERR().
---
This patch depends on Thomas Petazzoni DT patches for mvsdio, which changed the order of resource allocation etc.
Simon
You are correct about the indentation. As suggested, i moved the disable_unprepare() inside if (host).
It should no longer be possible to have imbalanced clk operations:
If mmc_alloc_host() fails, it goes to out. host is NULL, no clk calls. All other goto out occur after the clk_get() and when host is !NULL. clk calls are then made when !IS_ERR(host->clk), keeping this balanced.
drivers/mmc/host/mvsdio.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/mmc/host/mvsdio.c +4 -4
@@ -818,14 +818,14 @@ out:mmc_gpio_free_ro (mmc);if (host->base)iounmap (host->base); +if (!IS_ERR (host->clk)) { +clk_disable_unprepare (host->clk); +clk_put (host->clk); + } }if (r)release_resource (r);if (mmc) -if (!IS_ERR_OR_NULL (host->clk)) { -clk_disable_unprepare (host->clk); -clk_put (host->clk); - }mmc_free_host (mmc);return ret;
Signature · 1 line
1.7.10.4
PATCH v2 1/1 mmc: mvsdio: Replace IS_ERR_OR_NULL() with IS_ERR()
Quoted 8 lines · Andrew Lunn
A NULL is a valid clk cookie, so we should not be tested with
IS_ERR_OR_NULL(). Replace it withIS_ERR().Signed-off-by Andrew Lunn <[email protected]>---
This patch depends on Thomas Petazzoni DT patches for mvsdio, which changed the order of resource allocation etc.
grrr, I'm seeing fixes depending on features :( Any way to avoid that?
In light of Russell's comments, shall I wait for a v3? All of Thomas' mvsdio work is now waiting on this. I'd like to get that in as early as possible.
thx,
Jason.
PATCH v2 1/1 mmc: mvsdio: Replace IS_ERR_OR_NULL() with IS_ERR()
Quoted 7 lines · Jason Cooper
Quoted 6 lines · Andrew Lunn
A NULL is a valid clk cookie, so we should not be tested with
IS_ERR_OR_NULL(). Replace it withIS_ERR().Signed-off-by Andrew Lunn <[email protected]>---
Quoted 8 lines
Quoted 2 lines
This patch depends on Thomas Petazzoni DT patches for mvsdio, which changed the order of resource allocation etc.
grrr, I'm seeing fixes depending on features :( Any way to avoid that?
In light of Russell's comments, shall I wait for a v3? All of Thomas' mvsdio work is now waiting on this. I'd like to get that in as early as possible.
Well, im guessing doing the fixes first will require changes to Thomas features.
Did you try putting Russells patch in first? How bad are the conflicts?
Andrew
PATCH 1/1 mmc: mvsdio: Replace IS_ERR_NULL() is with IS_ERR()
In reply to Andrew LunnQuoted 13 lines · Andrew Lunn
A NULL is a valid clk cookie, so we should not be tested with
IS_ERR_NULL(). Replace it withIS_ERR().Signed-off-by Andrew Lunn <[email protected]>--- drivers/mmc/host/mvsdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/mmc/host/mvsdio.c +0
-0
drivers/mmc/host/mvsdio.c out: · line 845
Quoted 8 lines
drivers/mmc/host/mvsdio.c +1
-1@@ -839,7 +839,7 @@ out:if (r)release_resource (r);if (mmc) -if (!IS_ERR_OR_NULL (host->clk)) { +if (!IS_ERR (host->clk)) {clk_disable_unprepare (host->clk);clk_put (host->clk); }
Actually, no.
1. I've already recently pointed out that C is not python, and this code is broken. (Look at the lack of missing braces around the code you've modified.)
2. host->clk _can_ be NULL here.
Here's the (untested) bare minimum needed to fix this driver:
drivers/mmc/host/mvsdio.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
drivers/mmc/host/mvsdio.c +6 -4
@@ -733,6 +733,7 @@ static int __init mvsd_probe(struct platform_device *pdev) host->dev = &pdev->dev; host->res = r; host->base_clock = mvsd_data->clock /2 ; + host->clk =ERR_PTR (-EINVAL ); mmc->ops = &mvsd_ops;
@@ -837,13 +838,14 @@ static int __init mvsd_probe(struct platform_device *pdev)iounmap (host->base); }if (r) -release_resource (r); -if (mmc) -if (!IS_ERR_OR_NULL (host->clk)) { +release_mem_resource (r->start,resource_size (r)); +if (mmc) { +if (!IS_ERR (host->clk)) {clk_disable_unprepare (host->clk);clk_put (host->clk); }mmc_free_host (mmc); + }return ret; }
@@ -866,7 +868,7 @@ static int __exit mvsd_remove(struct platform_device *pdev)del_timer_sync (&host->timer);mvsd_power_down (host);iounmap (host->base); -release_resource (host->res); + release_mem_resource(host->res->start, resource_size(host->res);if (!IS_ERR (host->clk)) {clk_disable_unprepare (host->clk);
But... we can do better than that. We have the devm_* APIs which will make this code simpler, and less prone to these kinds of silly errors. I've left the request_irq()s alone because it isn't obvious to me that there isn't a reason for releasing the IRQ at a particular point in the cleanup, and there's no sign that IRQs may be asked on the device itself.
Note also that I found another bug in this driver - it requests a region 1K in size, but ioremaps 4K. That's just wrong. But anyway, the whole issue goes away with devm_request_and_ioremap().
What remains? The assumption that GPIO0 means "no GPIO" rather than testing with gpio_valid() and the IRQ stuff.
drivers/mmc/host/mvsdio.c | 61 +++++++++++++++----------------------------- 1 files changed, 21 insertions(+), 40 deletions(-)
drivers/mmc/host/mvsdio.c +21 -40
132 lines not on this page · raw message
PATCH 1/1 mmc: mvsdio: Replace IS_ERR_NULL() is with IS_ERR()
Dear Russell King - ARM Linux,
Quoted 2 lines · Russell King - ARM Linux
What remains? The assumption that GPIO0 means "no GPIO" rather than testing with
gpio_valid()and the IRQ stuff.
Patches that I have written have been merged by Jason Cooper and while adding the DT binding for this driver, it also converts it to use the MMC GPIO helpers from drivers/mmc/core/slot-gpio.c, which makes this whole GPIO initialization/cleanup a lot simpler.
See:
http://git.infradead.org/users/jcooper/linux.git/commitdiff/c3833fbb117bb1a547d29b27a0de4418fa2d5a5a
http://git.infradead.org/users/jcooper/linux.git/commitdiff/e60a21ed4edb7f33010ab21cefcb20666a9bc7d7
Also, note that Andrew Lunn has sent a v2 of his patch:
Subject: [PATCH v2] mmc: mvsdio: Replace IS_ERR_OR_NULL() with IS_ERR()
Date: Fri, 11 Jan 2013 08:27:52 +0100
Best regards,
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH 1/1 mmc: mvsdio: Replace IS_ERR_NULL() is with IS_ERR()
Quoted 11 lines · Thomas Petazzoni
Dear Russell King - ARM Linux,
Quoted 2 lines · Russell King - ARM Linux
What remains? The assumption that GPIO0 means "no GPIO" rather than testing with
gpio_valid()and the IRQ stuff.Patches that I have written have been merged by Jason Cooper and while adding the DT binding for this driver, it also converts it to use the MMC GPIO helpers from
drivers/mmc/core/slot-gpio.c, which makes this whole GPIO initialization/cleanup a lot simpler.
Please also fix the whole raft of other crap in this driver too, as I've highlighted previously and in these patches.