avorontsov at ru
Jun 5, 2008, 10:10 AM
Post #1 of 4
(218 views)
Permalink
|
|
[PATCH] mmc: toughen get_ro() and get_cd() return values
|
|
For the sake of safety, document that drivers should return only
1 or 0 from the get_ro() and get_cd() callbacks. Also document context
in which these callbacks should be executed.
wbsd driver modified to comply with this requirement.
Also, fix mmc_spi driver to not return raw values from the platform
get_cd hook (oops).
Suggested-by: Marc Pignat <marc.pignat[at]hevs.ch>
Signed-off-by: Anton Vorontsov <avorontsov[at]ru.mvista.com>
---
On Thu, Jun 05, 2008 at 05:58:59PM +0200, Marc Pignat wrote:
[...]
> > > * get_ro will return:
> > > * 0 for a read/write card
> > > * 1 for a read-only card
> >
> > This isn't always practical. For example, host is using u8 register for
> > the status, so it might safely return u8 & mask, that will always fit
> > into int. Or very smart/adventurous authors might be aware that, for the
> > particular host, mask's bit isn't last, and safely do uXX & mask. :-)
> >
> > The above is weak argument of course, since it is about optimization.
>
> Ack, we will gain at most 1-4 assembly instructions, in a function that
> is unlikely to be called more than once a second.
>
> >
> > As an counter-evidence, the strict scheme that you described probably
> > less error prone. But is it? To implement it we'll need something like
> > WARN_ON(ret > 0 && ret != 1) to catch erroneous users. Take a closer
> > look though, will it catch uXX & lastbit case? Nope. :-)
>
> WARN_ON(ret > 0 && ret != 1 || ret == INT_MIN) will do ;)
>
> I agree with you once more, I never thinked about a runtime check.
>
> I don't really want to see a WARN_ON(foo) after each call to get_ro or get_cd.
>
> But I'm sure if we specify "give me a positive value when a card is detected",
> someone will write gpio & bit, and three years later, someone will fall in
> the (gpio & lastbit < 0 case).
>
> So we should specify: "give me 1 whan a card is present, 0 when not, -ENOSYS if
> you don't know and a negative errno when something else goes wrong".
Well, ok.
Pierre, I see you didn't yet pushed out the mmc tree, so.. would you
prefer this patch folded into 0/3 series and resent?
drivers/mmc/host/mmc_spi.c | 2 +-
drivers/mmc/host/wbsd.c | 2 +-
include/linux/mmc/host.h | 16 ++++++++++++++--
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 85d9853..4e82f64 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1139,7 +1139,7 @@ static int mmc_spi_get_cd(struct mmc_host *mmc)
struct mmc_spi_host *host = mmc_priv(mmc);
if (host->pdata && host->pdata->get_cd)
- return host->pdata->get_cd(mmc->parent);
+ return !!host->pdata->get_cd(mmc->parent);
return -ENOSYS;
}
diff --git a/drivers/mmc/host/wbsd.c b/drivers/mmc/host/wbsd.c
index be624a0..9283b85 100644
--- a/drivers/mmc/host/wbsd.c
+++ b/drivers/mmc/host/wbsd.c
@@ -939,7 +939,7 @@ static int wbsd_get_ro(struct mmc_host *mmc)
spin_unlock_bh(&host->lock);
- return csr & WBSD_WRPT;
+ return !!(csr & WBSD_WRPT);
}
static const struct mmc_host_ops wbsd_ops = {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ef3b773..753b723 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -56,8 +56,20 @@ struct mmc_host_ops {
* since underlaying controller might implement them in an expensive
* and/or slow way.
*
- * .get_ro and .get_cd should return >= 0 for their logical values,
- * or negative errno value in case of error.
+ * Also note that these functions might sleep, so don't call them
+ * in the atomic contexts!
+ *
+ * Return values for the get_ro callback should be:
+ * 0 for a read/write card
+ * 1 for a read-only card
+ * -ENOSYS when not supported (equal to NULL callback)
+ * or a negative errno value when something bad happened
+ *
+ * Return values for the get_ro callback should be:
+ * 0 for a absent card
+ * 1 for a present card
+ * -ENOSYS when not supported (equal to NULL callback)
+ * or a negative errno value when something bad happened
*/
void (*set_ios)(struct mmc_host *host, struct mmc_ios *ios);
int (*get_ro)(struct mmc_host *host);
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
|
