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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
