Re: [PATCH] mmc: check EXT_CSD_PARTITION_SETTING_COMPLETED before creating partitions

[Ulf Hansson]

On 14 August 2014 15:27, GrÃgory Soutadà <gsoutade@xxxxxxxxxxx> wrote:
> Le 14/08/2014 13:46, Ulf Hansson a Ãcrit :
>> On 13 August 2014 11:20, GrÃgory Soutadà <gsoutade@xxxxxxxxxxx> wrote:
>>> Le 13/08/2014 10:36, Ulf Hansson a Ãcrit :
>>>> On 17 July 2014 16:57, GrÃgory Soutadà <gsoutade@xxxxxxxxxxx> wrote:
>>>>> Create MMC general purpose partitions only if
>>>>> EXT_CSD_PARTITION_SETTING_COMPLETED bit is set.
>>>>> Some tools may set general purpose partition size(s) but fail or stop
>>>>> without finalize.
>>>>> Another case is to set invalid partition size(s).
>>>>>
>>>>> Signed-off-by: GrÃgory Soutadà <gsoutade@xxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/mmc/core/mmc.c  |   15 +++++++++++----
>>>>>  include/linux/mmc/mmc.h |    2 ++
>>>>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>>>>
>>>>> From commit b6603fe574af289dbe9eb9fb4c540bca04f5a053 in master linux tree.
>>>>>
>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>> index 793c6f7..b9fe211 100644
>>>>> --- a/drivers/mmc/core/mmc.c
>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>> @@ -471,10 +471,17 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>>>>                                 ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3];
>>>>>                                 part_size *= (size_t)(hc_erase_grp_sz *
>>>>>                                         hc_wp_grp_sz);
>>>>> -                               mmc_part_add(card, part_size << 19,
>>>>> -                                       EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
>>>>> -                                       "gp%d", idx, false,
>>>>> -                                       MMC_BLK_DATA_AREA_GP);
>>>>> +                               if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED] &
>>>>> +                               EXT_CSD_PART_SETTING_COMPLETED) {
>>>>
>>>> Some minor comments here.
>>>>
>>>> I think you could move this if statement up and into the previous "if"
>>>> where we check for "ext_csd[EXT_CSD_PARTITION_SUPPORT] &
>>>> EXT_CSD_PART_SUPPORT_PART_EN".
>>>>
>>>> Additionally, please run checkpatch.
>>>>
>>>> Kind regards
>>>> Uffe
>>>
>>> Hello,
>>>
>>> I didn't put the if statement above to warn user in case of bad partitioning.
>>> I don't want the kernel to silently ignore this error.
>>
>> Fair enough.
>>
>> Still I am wondering whether hc_erase_grp_sz, hc_wp_grp_sz and
>> enhanced_area_en should be updated if
>> EXT_CSD_PARTITION_SETTING_COMPLETED isn't set.  That's the case in
>> your patch.
>
> I was focused on partitions and I didn't pay attention on enhanced area.
>
> JEDEC says that partitioning implies :
> * Configure general purpose partitions attributes and sizes
> * Configure enhanced user area offset, attributes and size
>
> and finally set EXT_CSD_PARTITION_SETTING_COMPLETED.
>
> Thus these two parts must checks for setting completed before
> computing values.
>
> Plus, "enhanced_area_en" attribute is set whether or not there is an
> enhanced area defined. I looked at the code, and the only usage of it
> is to set EXT_CSD_ERASE_GROUP_DEF and compute erase size again.
> I suggest using "partition_setting_completed" identifier which is common
> to the two functions and requires EXT_CSD_ERASE_GROUP_DEF to be set.
>
> If you're ok with that, I'll submit another patch.

Seems reasonable. Please send a v2.

Kind regards
Uffe

>
>
> Best regards
> GrÃgory SoutadÃ
>
>>
>>>
>>> checkpatch has been run before sending this patch, I know there are two lines
>>> with two extra characters, but names used here are quite long. I hope it will
>>> not block upstream inclusion.
>>
>> The mmc_read_ext_csd() is not one of the nicest piece of code, but for
>> sure we should not move on making it worse. If you need to move code
>> into separate function to prevent checkpatch warnings, please do so.
>>
>> Kind regards
>> Uffe
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/