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/
 | 
Pages: 1
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.