On Wed, Jul 7, 2010 at 7:17 AM, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:From 7f01ad3c4be6ec09318176db12db66f353b526e0 Mon Sep 17 00:00:00 2001
SD/MMC cards tend to support an erase operation. In addition,
eMMC v4.4 cards can support secure erase, trim and secure trim
operations that are all variants of the basic erase command.
This is great. I am interested primarily in SD media.
Please forgive my naive perspective: it seems that with the features
enabled by this patchset and a filesystem that is capable of issuing
erase block commands, the wear-leveling on SD media will be improved
-- much like with CF TRIM commands. Do you also think that is the
case? I would be very interested in hearing your expert opinion on
this.
I have a couple comments regarding mostly the SD support introduced in
this patch. Patches 2..5 of 5 seem fine to me but I'm not sure I'm
qualified to add acks or reviewed-by's.
+int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
+ unsigned int arg)
+{
+ unsigned int rem, to = from + nr;
+
+ if (!(card->host->caps & MMC_CAP_ERASE) ||
+ !(card->csd.cmdclass & CCC_ERASE))
+ return -EOPNOTSUPP;
+
+ if (!card->erase_size)
+ return -EOPNOTSUPP;
+
+ if (mmc_card_sd(card) && arg != MMC_ERASE_ARG)
+ return -EOPNOTSUPP;
+
+ if ((arg & MMC_SECURE_ARGS) &&
+ !(card->ext_csd.sec_feature_support & EXT_CSD_SEC_ER_EN))
+ return -EOPNOTSUPP;
+
+ if ((arg & MMC_TRIM_ARGS) &&
+ !(card->ext_csd.sec_feature_support & EXT_CSD_SEC_GB_CL_EN))
+ return -EOPNOTSUPP;
+
+int mmc_can_trim(struct mmc_card *card)
+{
+ if (card->ext_csd.sec_feature_support & EXT_CSD_SEC_GB_CL_EN)
+ return 1;
+ return 0;
+}
+EXPORT_SYMBOL(mmc_can_trim);
It looks like mmc_can_trim(card) would return true when
mmc_card_sd(card) is true;
but the mmc_erase function will return
-EOPNOTSUPP in such a case. I think that this results in the
mmc_blk_issue_discard_rq function (from 2/5) also returning
-EOPNOTSUPP whenever mmc_card_sd(card) is true:
From 2/5:+ if (mmc_can_trim(card))
+ arg = MMC_TRIM_ARG;
+ else
+ arg = MMC_ERASE_ARG;
+
+ err = mmc_erase(card, from, nr, arg);
Also, there is some duplication between the sec_feature_support
checking in mmc_erase and in the mmc_can* functions; If mmc_erase
could call the mmc_can_* functions then the the bit-checking logic
could be centralized.
/*
+ * Fetch and process SD Status register.
+ */
+static int mmc_read_ssr(struct mmc_card *card)
+{
It looks like the conventional function prefix for SD-specific
functions in the rest of this file is mmc_sd_ ; 'mmc_read_ssr' ->
'mmc_sd_read_ssr' or -> 'mmc_read_sd_sr' perhaps?
+ ssr = kmalloc(64, GFP_KERNEL);
Why '64' instead of 'sizeof(*ssr)' ?
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca