Prev: [PATCH 02/10] Security: Add Hook to test if the particular xattr is part of a MAC model.
Next: [PATCH 01/10] Security: Add hook to calculate context based on a negative dentry.
From: Ben Gardiner on 7 Jul 2010 11:10 On Wed, Jul 7, 2010 at 7:17 AM, Adrian Hunter <adrian.hunter(a)nokia.com> 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-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Gardiner on 7 Jul 2010 16:50
On Wed, Jul 7, 2010 at 3:05 PM, Adrian Hunter <adrian.hunter(a)nokia.com> wrote: > Ben Gardiner wrote: >> >> On Wed, Jul 7, 2010 at 7:17 AM, Adrian Hunter <adrian.hunter(a)nokia.com> >> 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 am sorry but I don't know. �Wear-levelling in cards tends to be kept > secret by the manufacturers. �However, it is not clear to me that cards > bother to record whether or not anything has been erased. � For example, > erase a card twice - it takes the same amount of time the second time > as the first time, whereas if the card knew it was already erased, why > wasn't the second time much quicker? No worries. I'm happy to hear your opinion anyways. Interesting observation re: erase time of cards, I assume that is "erase" as in the SD erase operations as proposed in this patch as opposed to erase as in 'mkfs'. >> >> 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; > > It will return false for SD. �card->ext_csd.sec_feature_support > is only used by MMC. Makes sense now, thanks. >>> /* >>> + * 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? > > Well there is also mmc_decode_*, and mmc_read_switch so the other > functions that do smilar things also do not follow that convention. Good point. >> >>> + � � � ssr = kmalloc(64, GFP_KERNEL); >> >> Why '64' instead of 'sizeof(*ssr)' ? > > sizeof(*ssr) is 4 Right -- my mistake :) I guess I was _thinking_ 16*sizeof(*ssr) or SSR_SIZE*sizeof(*ssr) instead of a magic number '64'. I see now that this wouldn't be the only kmalloc of a magic number in sd.c -- so I'll stop being so picky. Reviewed-by: Ben Gardiner <bengardiner(a)nanometrics.ca> --- Ben Gardiner Nanometrics Inc. http://www.nanometrics.ca -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |