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

[Ulf Hansson]

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.

>
> 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/