mmc: check EXT_CSD_PARTITION_SETTING_COMPLETED before creating partitions
PATCH 1/1 mmc: check EXT_CSD_PARTITION_SETTING_COMPLETED before creating partitions
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).
--- 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.
drivers/mmc/core/mmc.c +11 -4
@@ -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 ) { +mmc_part_add (card, part_size <<19 , +EXT_CSD_PART_CONFIG_ACC_GP0 + idx, +"gp%d" , idx, false, +MMC_BLK_DATA_AREA_GP ); + }else { +pr_warn ("%s: has partition size defined, but setting is not complete\n" , +mmc_hostname (card->host)); +break ; + } } } card->ext_csd.sec_trim_mult =
include/linux/mmc/mmc.h +2 -0
@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */
@@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
Signature · 1 line
1.7.0.4
Quoted 19 lines · Grégory Soutadé
Create MMC general purpose partitions only if
EXT_CSD_PARTITION_SETTING_COMPLETEDbit 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é <[email protected]>--- drivers/mmc/core/mmc.c | 15 +++++++++++---- include/linux/mmc/mmc.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)From commit
b6603fe574af289dbe9eb9fb4c540bca04f5a053in master linux tree.drivers/mmc/core/mmc.c +0
-0
drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 475
Quoted 9 lines
drivers/mmc/core/mmc.c +2
-4@@ -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
Quoted 34 lines
drivers/mmc/core/mmc.c +9
-0+mmc_part_add (card, part_size <<19 , +EXT_CSD_PART_CONFIG_ACC_GP0 + idx, +"gp%d" , idx, false, +MMC_BLK_DATA_AREA_GP ); + }else { +pr_warn ("%s: has partition size defined, but setting is not complete\n" , +mmc_hostname (card->host)); +break ; + } } } card->ext_csd.sec_trim_mult =include/linux/mmc/mmc.h +2
-1@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0) --1.7.0.4
Quoted 32 lines · Ulf Hansson
Quoted 19 lines · Grégory Soutadé
Create MMC general purpose partitions only if
EXT_CSD_PARTITION_SETTING_COMPLETEDbit 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é <[email protected]>--- drivers/mmc/core/mmc.c | 15 +++++++++++---- include/linux/mmc/mmc.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)From commit
b6603fe574af289dbe9eb9fb4c540bca04f5a053in master linux tree.drivers/mmc/core/mmc.c +0
-0drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 475
Quoted 9 lines
drivers/mmc/core/mmc.c +2
-4@@ -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.
Quoted 8 lines
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.
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.
Best regards
Grégory Soutadé
Quoted 36 lines
Quoted 35 lines
drivers/mmc/core/mmc.c +9
-0+mmc_part_add (card, part_size <<19 , +EXT_CSD_PART_CONFIG_ACC_GP0 + idx, +"gp%d" , idx, false, +MMC_BLK_DATA_AREA_GP ); + }else { +pr_warn ("%s: has partition size defined, but setting is not complete\n" , +mmc_hostname (card->host)); +break ; + } } } card->ext_csd.sec_trim_mult =include/linux/mmc/mmc.h +2
-1@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0) --1.7.0.4
Quoted 39 lines · Grégory Soutadé
Quoted 38 lines · Ulf Hansson
Quoted 19 lines · Grégory Soutadé
Create MMC general purpose partitions only if
EXT_CSD_PARTITION_SETTING_COMPLETEDbit 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é <[email protected]>--- drivers/mmc/core/mmc.c | 15 +++++++++++---- include/linux/mmc/mmc.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)From commit
b6603fe574af289dbe9eb9fb4c540bca04f5a053in master linux tree.drivers/mmc/core/mmc.c +0
-0drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 475
Quoted 9 lines
drivers/mmc/core/mmc.c +2
-4@@ -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.
Quoted 7 lines
Quoted 2 lines
Kind regards
UffeHello,
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.
Quoted 4 lines
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
Quoted 48 lines · Ulf Hansson
Quoted 39 lines · Grégory Soutadé
Quoted 38 lines · Ulf Hansson
Quoted 19 lines · Grégory Soutadé
Create MMC general purpose partitions only if
EXT_CSD_PARTITION_SETTING_COMPLETEDbit 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é <[email protected]>--- drivers/mmc/core/mmc.c | 15 +++++++++++---- include/linux/mmc/mmc.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)From commit
b6603fe574af289dbe9eb9fb4c540bca04f5a053in master linux tree.drivers/mmc/core/mmc.c +0
-0drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 475
Quoted 9 lines
drivers/mmc/core/mmc.c +2
-4@@ -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.
Quoted 7 lines
Quoted 2 lines
Kind regards
UffeHello,
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.
Quoted 6 lines
Fair enough.
Still I am wondering whether
hc_erase_grp_sz,hc_wp_grp_szandenhanced_area_enshould be updated ifEXT_CSD_PARTITION_SETTING_COMPLETEDisn'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.
Best regards
Grégory Soutadé
Quoted 13 lines
Quoted 5 lines
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
Quoted 67 lines · Grégory Soutadé
Quoted 48 lines · Ulf Hansson
Quoted 39 lines · Grégory Soutadé
Quoted 38 lines · Ulf Hansson
Quoted 19 lines · Grégory Soutadé
Create MMC general purpose partitions only if
EXT_CSD_PARTITION_SETTING_COMPLETEDbit 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é <[email protected]>--- drivers/mmc/core/mmc.c | 15 +++++++++++---- include/linux/mmc/mmc.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)From commit
b6603fe574af289dbe9eb9fb4c540bca04f5a053in master linux tree.drivers/mmc/core/mmc.c +0
-0drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 475
Quoted 9 lines
drivers/mmc/core/mmc.c +2
-4@@ -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.
Quoted 7 lines
Quoted 2 lines
Kind regards
UffeHello,
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.
Quoted 6 lines
Fair enough.
Still I am wondering whether
hc_erase_grp_sz,hc_wp_grp_szandenhanced_area_enshould be updated ifEXT_CSD_PARTITION_SETTING_COMPLETEDisn'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 sizeand finally set
EXT_CSD_PARTITION_SETTING_COMPLETED.Thus these two parts must checks for setting completed before computing values.
Quoted 7 lines
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 setEXT_CSD_ERASE_GROUP_DEFand compute erase size again. I suggest using "partition_setting_completed" identifier which is common to the two functions and requiresEXT_CSD_ERASE_GROUP_DEFto be set.If you're ok with that, I'll submit another patch.
Seems reasonable. Please send a v2.
Kind regards
Uffe
Quoted 19 lines
Best regards
Grégory SoutadéQuoted 13 lines
Quoted 5 lines
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
PATCH v2 2/2 mmc: check EXT_CSD_PARTITION_SETTING_COMPLETED before creating partitions
In reply to Grégory SoutadéChecks EXT_CSD_PARTITION_SETTING_COMPLETED bit before computing enhanced user area and adding mmc partitions (as described in JEDEC standard).
--- drivers/mmc/core/mmc.c | 167 +++++++++++++++++++++++++++--------------------- 1 file changed, 95 insertions(+), 72 deletions(-)
Changelog v2:
Move code for user area and general purpose partitions into functions.
drivers/mmc/core/mmc.c +95 -72
@@ -298,6 +298,99 @@ static void mmc_select_card_type(struct mmc_card *card) card->mmc_avail_type = avail_type; } +static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) +{ +u8 hc_erase_grp_sz, hc_wp_grp_sz; + +/* +* Enhanced area feature support -- check whether the eMMC +* card has the Enhanced area enabled. If so, export enhanced +* area offset and size to user by adding sysfs interface. +*/ +if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && + (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { +if (card->ext_csd.partition_setting_completed) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +pr_warn ("%s: defines enhanced area without partition setting complete\n" , +mmc_hostname (card->host)); + } + }else { +/* +* If the enhanced area is not enabled, disable these +* device attributes. +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ; + } +} + +static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) +{ +int idx; +u8 hc_erase_grp_sz, hc_wp_grp_sz; +unsigned int part_size; + +/* +* General purpose partition feature support -- +* If ext_csd has the size of general purpose partitions, +* set size, part_cfg, partition name in mmc_part. +*/ +if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & +EXT_CSD_PART_SUPPORT_PART_EN ) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + +for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { +if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) +continue ; + +if (card->ext_csd.partition_setting_completed ==0 ) { +pr_warn ("%s: has partition size defined without partition complete\n" , +mmc_hostname (card->host)); +break ; + } + + part_size = + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] + <<16 ) + + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] + <<8 ) + + 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 ); + } + } +} +/* * Decode extended CSD. */
@@ -305,7 +398,6 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) {int err =0 , idx;unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;BUG_ON (!card);
@@ -409,80 +501,11 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) }else { card->ext_csd.partition_setting_completed =0 ; } -/* -* Enhanced area feature support -- check whether the eMMC -* card has the Enhanced area enabled. If so, export enhanced -* area offset and size to user by adding sysfs interface. -*/ -if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && - (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; - } +mmc_manage_enhanced_area (card, ext_csd); -/* -* General purpose partition feature support -- -* If ext_csd has the size of general purpose partitions, -* set size, part_cfg, partition name in mmc_part. -*/ -if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & -EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } +mmc_manage_gp_partitions (card, ext_csd); -for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { -if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) -continue ; - part_size = - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] - <<16 ) + - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] - <<8 ) + - 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 ); - } - } card->ext_csd.sec_trim_mult = ext_csd[EXT_CSD_SEC_TRIM_MULT ]; card->ext_csd.sec_erase_mult =
Signature · 1 line
1.7.9.5
PATCH v2 1/2 mmc: check EXT_CSD_PARTITION_SETTING_COMPLETED before creating partitions
In reply to Grégory SoutadéReplace "enhanced_area_en" attribute by "partition_setting_completed". It was used whether or not enhanced user area is defined and without checks of EXT_CSD_PARTITION_SETTING_COMPLETED bit.
--- drivers/mmc/core/mmc.c | 8 +++++++- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-)
Quoted 1 line
From commit
33caee39925b887a99a2400dc5c980097c3573f9in master linux tree.
drivers/mmc/core/mmc.c +7 -1
@@ -403,6 +403,12 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + }/* * Enhanced area feature support -- check whether the eMMC * card has the Enhanced area enabled. If so, export enhanced
@@ -1309,7 +1315,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,
include/linux/mmc/card.h +1 -1
@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */
include/linux/mmc/mmc.h +2 -0
@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */
@@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
Signature · 1 line
1.7.9.5
Quoted 5 lines · Grégory Soutadé
Checks
EXT_CSD_PARTITION_SETTING_COMPLETEDbit before computing enhanced user area and adding mmc partitions (as described in JEDEC standard).Signed-off-by Grégory Soutadé <[email protected]>
Hi Grégory,
First, could you rename patches so they actually describe what they actually do. Currently both of them have the same header. Second, I really appreciate if there could be some version history of the patches since it helps review process.
Could you please resend them?
Kind regards
Uffe
Quoted 205 lines
--- drivers/mmc/core/mmc.c | 167 +++++++++++++++++++++++++++--------------------- 1 file changed, 95 insertions(+), 72 deletions(-)Changelog v2:
Move code for user area and general purpose partitions into functions.drivers/mmc/core/mmc.c +95
-73@@ -298,6 +298,99 @@ static void mmc_select_card_type(struct mmc_card *card) card->mmc_avail_type = avail_type; } +static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) +{ +u8 hc_erase_grp_sz, hc_wp_grp_sz; + +/* +* Enhanced area feature support -- check whether the eMMC +* card has the Enhanced area enabled. If so, export enhanced +* area offset and size to user by adding sysfs interface. +*/ +if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && + (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { +if (card->ext_csd.partition_setting_completed) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +pr_warn ("%s: defines enhanced area without partition setting complete\n" , +mmc_hostname (card->host)); + } + }else { +/* +* If the enhanced area is not enabled, disable these +* device attributes. +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ; + } +} + +static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) +{ +int idx; +u8 hc_erase_grp_sz, hc_wp_grp_sz; +unsigned int part_size; + +/* +* General purpose partition feature support -- +* If ext_csd has the size of general purpose partitions, +* set size, part_cfg, partition name in mmc_part. +*/ +if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & +EXT_CSD_PART_SUPPORT_PART_EN ) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + +for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { +if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) +continue ; + +if (card->ext_csd.partition_setting_completed ==0 ) { +pr_warn ("%s: has partition size defined without partition complete\n" , +mmc_hostname (card->host)); +break ; + } + + part_size = + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] + <<16 ) + + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] + <<8 ) + + 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 ); + } + } +} +/* * Decode extended CSD. */ @@ -305,7 +398,6 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) {int err =0 , idx;unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;BUG_ON (!card);@@ -409,80 +501,11 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) }else { card->ext_csd.partition_setting_completed =0 ; } -/* -* Enhanced area feature support -- check whether the eMMC -* card has the Enhanced area enabled. If so, export enhanced -* area offset and size to user by adding sysfs interface. -*/ -if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && - (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; - } +mmc_manage_enhanced_area (card, ext_csd); -/* -* General purpose partition feature support -- -* If ext_csd has the size of general purpose partitions, -* set size, part_cfg, partition name in mmc_part. -*/ -if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & -EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } +mmc_manage_gp_partitions (card, ext_csd); -for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { -if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) -continue ; - part_size = - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] - <<16 ) + - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] - <<8 ) + - 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 ); - } - } card->ext_csd.sec_trim_mult = ext_csd[EXT_CSD_SEC_TRIM_MULT ]; card->ext_csd.sec_erase_mult = --1.7.9.5
PATCH v3 2/2 mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED bit before partitions computation
In reply to Ulf HanssonChecks EXT_CSD_PARTITION_SETTING_COMPLETED bit before computing enhanced user area offset and size, and adding mmc general purpose partitions.
The two needs EXT_CSD_PARTITION_SETTING_COMPLETED bit be set to be valid (as described in JEDEC standard).
--- drivers/mmc/core/mmc.c | 167 +++++++++++++++++++++++++++--------------------- 1 file changed, 95 insertions(+), 72 deletions(-)
Changelog v2:
Move code for user area and general purpose partitions into functions.
drivers/mmc/core/mmc.c +95 -72
@@ -298,6 +298,99 @@ static void mmc_select_card_type(struct mmc_card *card) card->mmc_avail_type = avail_type; } +static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) +{ +u8 hc_erase_grp_sz, hc_wp_grp_sz; + +/* +* Enhanced area feature support -- check whether the eMMC +* card has the Enhanced area enabled. If so, export enhanced +* area offset and size to user by adding sysfs interface. +*/ +if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && + (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { +if (card->ext_csd.partition_setting_completed) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +pr_warn ("%s: defines enhanced area without partition setting complete\n" , +mmc_hostname (card->host)); + } + }else { +/* +* If the enhanced area is not enabled, disable these +* device attributes. +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ; + } +} + +static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) +{ +int idx; +u8 hc_erase_grp_sz, hc_wp_grp_sz; +unsigned int part_size; + +/* +* General purpose partition feature support -- +* If ext_csd has the size of general purpose partitions, +* set size, part_cfg, partition name in mmc_part. +*/ +if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & +EXT_CSD_PART_SUPPORT_PART_EN ) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + +for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { +if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) +continue ; + +if (card->ext_csd.partition_setting_completed ==0 ) { +pr_warn ("%s: has partition size defined without partition complete\n" , +mmc_hostname (card->host)); +break ; + } + + part_size = + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] + <<16 ) + + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] + <<8 ) + + 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 ); + } + } +} +/* * Decode extended CSD. */
@@ -305,7 +398,6 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) {int err =0 , idx;unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;BUG_ON (!card);
@@ -409,80 +501,11 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) }else { card->ext_csd.partition_setting_completed =0 ; } -/* -* Enhanced area feature support -- check whether the eMMC -* card has the Enhanced area enabled. If so, export enhanced -* area offset and size to user by adding sysfs interface. -*/ -if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && - (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; - } +mmc_manage_enhanced_area (card, ext_csd); -/* -* General purpose partition feature support -- -* If ext_csd has the size of general purpose partitions, -* set size, part_cfg, partition name in mmc_part. -*/ -if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & -EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } +mmc_manage_gp_partitions (card, ext_csd); -for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { -if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) -continue ; - part_size = - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] - <<16 ) + - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] - <<8 ) + - 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 ); - } - } card->ext_csd.sec_trim_mult = ext_csd[EXT_CSD_SEC_TRIM_MULT ]; card->ext_csd.sec_erase_mult =
--
1.7.9.5
PATCH v3 1/2 mmc: Replace ext_csd "enhanced_area_en" attribute by "partition_setting_completed"
In reply to Ulf HanssonReplace ext_csd "enhanced_area_en" attribute by "partition_setting_completed". It was used whether or not enhanced user area is defined and without checks of EXT_CSD_PARTITION_SETTING_COMPLETED bit.
--- drivers/mmc/core/mmc.c | 8 +++++++- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-)
Quoted 1 line
From commit
33caee39925b887a99a2400dc5c980097c3573f9in master linux tree.
drivers/mmc/core/mmc.c +7 -1
@@ -403,6 +403,12 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + }/* * Enhanced area feature support -- check whether the eMMC * card has the Enhanced area enabled. If so, export enhanced
@@ -1309,7 +1315,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,
include/linux/mmc/card.h +1 -1
@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */
include/linux/mmc/mmc.h +2 -0
@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */
@@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
--
1.7.9.5
Quoted 101 lines · Ulf Hansson
Quoted 67 lines · Grégory Soutadé
Quoted 48 lines · Ulf Hansson
Quoted 39 lines · Grégory Soutadé
Quoted 38 lines · Ulf Hansson
Quoted 19 lines · Grégory Soutadé
Create MMC general purpose partitions only if
EXT_CSD_PARTITION_SETTING_COMPLETEDbit 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é <[email protected]>--- drivers/mmc/core/mmc.c | 15 +++++++++++---- include/linux/mmc/mmc.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)From commit
b6603fe574af289dbe9eb9fb4c540bca04f5a053in master linux tree.drivers/mmc/core/mmc.c +0
-0drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 475
Quoted 9 lines
drivers/mmc/core/mmc.c +2
-4@@ -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.
Quoted 7 lines
Quoted 2 lines
Kind regards
UffeHello,
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.
Quoted 6 lines
Fair enough.
Still I am wondering whether
hc_erase_grp_sz,hc_wp_grp_szandenhanced_area_enshould be updated ifEXT_CSD_PARTITION_SETTING_COMPLETEDisn'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 sizeand finally set
EXT_CSD_PARTITION_SETTING_COMPLETED.Thus these two parts must checks for setting completed before computing values.
Quoted 7 lines
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 setEXT_CSD_ERASE_GROUP_DEFand compute erase size again. I suggest using "partition_setting_completed" identifier which is common to the two functions and requiresEXT_CSD_ERASE_GROUP_DEFto be set.If you're ok with that, I'll submit another patch.
Seems reasonable. Please send a v2.
Kind regards
UffeQuoted 19 lines
Best regards
Grégory SoutadéQuoted 13 lines
Quoted 5 lines
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
Re: [PATCH v3 0002/0002] mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED bit before partitions computation
In reply to Grégory SoutadéQuoted 6 lines · Grégory Soutadé
Checks
EXT_CSD_PARTITION_SETTING_COMPLETEDbit before computing enhanced user area offset and size, and adding mmc general purpose partitions.
The two needsEXT_CSD_PARTITION_SETTING_COMPLETEDbit be set to be valid (as described in JEDEC standard).
Quoted 8 lines
Signed-off-by Grégory Soutadé <[email protected]>--- drivers/mmc/core/mmc.c | 167 +++++++++++++++++++++++++++--------------------- 1 file changed, 95 insertions(+), 72 deletions(-)Changelog v2:
Move code for user area and general purpose partitions into functions.
Overall I like the end-result of this patch.
What I don't like is that you move code and fix bugs in same patch. Could possibly, split this up into two patches?
Kind regards
Uffe
PATCH v4 2/3 mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
In reply to Grégory SoutadéReplace ext_csd "enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks of EXT_CSD_PARTITION_SETTING_COMPLETED bit.
--- drivers/mmc/core/mmc.c | 9 ++++++++- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)
drivers/mmc/core/mmc.c +8 -1
@@ -488,6 +488,13 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + } +mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);
@@ -1324,7 +1331,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,
include/linux/mmc/card.h +1 -1
@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */
include/linux/mmc/mmc.h +2 -0
@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */
@@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
Signature · 1 line
1.7.9.5
PATCH v4 1/3 mmc: Move code that manages user area and gp partitions into functions
In reply to Grégory SoutadéMove code that manages user area and general purpose
partitions into functions.
--- drivers/mmc/core/mmc.c | 162 ++++++++++++++++++++++++++---------------------- 1 file changed, 89 insertions(+), 73 deletions(-)
Quoted 1 line
From commit
480cadc2b7e0fa2bbab20141efb547dfe0c3707cin master linux tree.
Changelog v3:
Move code BEFORE fixing bugs.
Changelog v2:
Move code for user area and general purpose partitions into functions.
drivers/mmc/core/mmc.c +89 -73
@@ -298,6 +298,93 @@ static void mmc_select_card_type(struct mmc_card *card) card->mmc_avail_type = avail_type; } +static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) +{ +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; + +/* +* Enhanced area feature support -- check whether the eMMC +* card has the Enhanced area enabled. If so, export enhanced +* area offset and size to user by adding sysfs interface. +*/ +if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && + (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +/* +* If the enhanced area is not enabled, disable these +* device attributes. +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ; + } +} + +static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) +{ +unsigned int part_size; +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +int idx; + +/* +* General purpose partition feature support -- +* If ext_csd has the size of general purpose partitions, +* set size, part_cfg, partition name in mmc_part. +*/ +if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & +EXT_CSD_PART_SUPPORT_PART_EN ) { +if (card->ext_csd.enhanced_area_en !=1 ) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; + } + +for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { +if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) +continue ; + part_size = + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] + <<16 ) + + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] + <<8 ) + + 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 ); + } + } +} +/* * Decode extended CSD. */
@@ -305,7 +392,6 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) {int err =0 , idx;unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;BUG_ON (!card);
@@ -402,80 +488,10 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { -/* -* Enhanced area feature support -- check whether the eMMC -* card has the Enhanced area enabled. If so, export enhanced -* area offset and size to user by adding sysfs interface. -*/ -if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && - (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +mmc_manage_enhanced_area (card, ext_csd); - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; - } +mmc_manage_gp_partitions (card, ext_csd); -/* -* General purpose partition feature support -- -* If ext_csd has the size of general purpose partitions, -* set size, part_cfg, partition name in mmc_part. -*/ -if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & -EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } - -for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { -if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) -continue ; - part_size = - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] - <<16 ) + - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] - <<8 ) + - 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 ); - } - } card->ext_csd.sec_trim_mult = ext_csd[EXT_CSD_SEC_TRIM_MULT ]; card->ext_csd.sec_erase_mult =
Signature · 1 line
1.7.9.5
Quoted 177 lines
Replace
ext_csd"enhanced_area_en" attribute by "partition_setting_completed". It was used whether or not enhanced user area is defined and without checks ofEXT_CSD_PARTITION_SETTING_COMPLETEDbit.Signed-off-by Grégory Soutadé <[email protected]>--- drivers/mmc/core/mmc.c | 8 +++++++- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-)Quoted 1 line
From commit
33caee39925b887a99a2400dc5c980097c3573f9in master linux tree.drivers/mmc/core/mmc.c +7
-1@@ -403,6 +403,12 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + }/* * Enhanced area feature support -- check whether the eMMC * card has the Enhanced area enabled. If so, export enhanced @@ -1309,7 +1315,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,include/linux/mmc/card.h +1
-1@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */ include/linux/mmc/mmc.h +2
-1@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0) --1.7.9.5
Quoted 101 lines · Ulf Hansson
Quoted 67 lines · Grégory Soutadé
Quoted 48 lines · Ulf Hansson
Quoted 39 lines · Grégory Soutadé
Quoted 38 lines · Ulf Hansson
Quoted 19 lines · Grégory Soutadé
Create MMC general purpose partitions only if
EXT_CSD_PARTITION_SETTING_COMPLETEDbit 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é <[email protected]>--- drivers/mmc/core/mmc.c | 15 +++++++++++---- include/linux/mmc/mmc.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)From commit
b6603fe574af289dbe9eb9fb4c540bca04f5a053in master linux tree.drivers/mmc/core/mmc.c +0
-0drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 475
Quoted 9 lines
drivers/mmc/core/mmc.c +2
-4@@ -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.
Quoted 7 lines
Quoted 2 lines
Kind regards
UffeHello,
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.
Quoted 6 lines
Fair enough.
Still I am wondering whether
hc_erase_grp_sz,hc_wp_grp_szandenhanced_area_enshould be updated ifEXT_CSD_PARTITION_SETTING_COMPLETEDisn'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 sizeand finally set
EXT_CSD_PARTITION_SETTING_COMPLETED.Thus these two parts must checks for setting completed before computing values.
Quoted 7 lines
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 setEXT_CSD_ERASE_GROUP_DEFand compute erase size again. I suggest using "partition_setting_completed" identifier which is common to the two functions and requiresEXT_CSD_ERASE_GROUP_DEFto be set.If you're ok with that, I'll submit another patch.
Seems reasonable. Please send a v2.
Kind regards
UffeQuoted 19 lines
Best regards
Grégory SoutadéQuoted 13 lines
Quoted 5 lines
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
PATCH v4 3/3 mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED before partitions computation
In reply to Grégory SoutadéChecks EXT_CSD_PARTITION_SETTING_COMPLETED bit before
computing enhanced user area offset and size, and adding mmc general purpose partitions. The two needs EXT_CSD_PARTITION_SETTING_COMPLETED bit be set to be valid (as described in JEDEC standard).
--- drivers/mmc/core/mmc.c | 84 +++++++++++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 40 deletions(-)
drivers/mmc/core/mmc.c +44 -40
@@ -300,7 +300,13 @@ static void mmc_select_card_type(struct mmc_card *card)static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) { -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +u8 hc_erase_grp_sz, hc_wp_grp_sz; + +/* +* Disable these attributes by default +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ;/* * Enhanced area feature support -- check whether the eMMC
@@ -309,44 +315,41 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd)*/ if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +if (card->ext_csd.partition_setting_completed) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +pr_warn ("%s: defines enhanced area without partition setting complete\n" , +mmc_hostname (card->host)); + } } }static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) { -unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;int idx; +u8 hc_erase_grp_sz, hc_wp_grp_sz; +unsigned int part_size;/* * General purpose partition feature support --
@@ -355,20 +358,21 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ];for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ])continue ; +if (card->ext_csd.partition_setting_completed ==0 ) { +pr_warn ("%s: has partition size defined without partition complete\n" , +mmc_hostname (card->host)); +break ; + } part_size = (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] << 16) +
Signature · 1 line
1.7.9.5
Re: [PATCH v4 0002/0003] mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
In reply to Grégory SoutadéQuoted 6 lines · Grégory Soutadé
Replace
ext_csd"enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks ofEXT_CSD_PARTITION_SETTING_COMPLETEDbit.Signed-off-by Grégory Soutadé <[email protected]>
This patch doesn't compile.
I also think you could work a bit on the commit messages in this patchset. Try to describe what the patches do and why. I may help you
- if you like.
Quoted 11 lines
--- drivers/mmc/core/mmc.c | 9 ++++++++- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)drivers/mmc/core/mmc.c +0
-0
drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 492
Quoted 5 lines
drivers/mmc/core/mmc.c +2
-0@@ -488,6 +488,13 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) {
No need for brackets if this if-else, please remove.
drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 500
Quoted 8 lines
drivers/mmc/core/mmc.c +5
-0+ card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + } +mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);
[...]
Kind regards
Uffe
PATCH v5 3/3 mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED before partitions computation
In reply to Grégory SoutadéChecks EXT_CSD_PARTITION_SETTING_COMPLETED bit before
computing enhanced user area offset and size, and adding mmc general purpose partitions. The two needs EXT_CSD_PARTITION_SETTING_COMPLETED bit be set to be valid (as described in JEDEC standard).
Warn user in case of misconfiguration.
--- drivers/mmc/core/mmc.c | 81 ++++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 37 deletions(-)
drivers/mmc/core/mmc.c +44 -37
@@ -300,7 +300,13 @@ static void mmc_select_card_type(struct mmc_card *card)static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) { -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +u8 hc_erase_grp_sz, hc_wp_grp_sz; + +/* +* Disable these attributes by default +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ;/* * Enhanced area feature support -- check whether the eMMC
@@ -309,43 +315,41 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd)*/ if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +if (card->ext_csd.partition_setting_completed) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +pr_warn ("%s: defines enhanced area without partition setting complete\n" , +mmc_hostname (card->host)); + } } }static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) { -unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;int idx; +u8 hc_erase_grp_sz, hc_wp_grp_sz; +unsigned int part_size;/* * General purpose partition feature support --
@@ -354,18 +358,21 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.partition_setting_completed) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - } + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ];for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ])continue ; +if (card->ext_csd.partition_setting_completed ==0 ) { +pr_warn ("%s: has partition size defined without partition complete\n" , +mmc_hostname (card->host)); +break ; + } part_size = (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] << 16) +
Signature · 1 line
1.7.9.5
PATCH v5 1/3 mmc: Move code that manages user area and gp partitions into functions
In reply to Grégory SoutadéMove code that manages user area and general purpose
partitions into functions.
--- drivers/mmc/core/mmc.c | 162 ++++++++++++++++++++++++++---------------------- 1 file changed, 89 insertions(+), 73 deletions(-)
drivers/mmc/core/mmc.c +89 -73
@@ -298,6 +298,93 @@ static void mmc_select_card_type(struct mmc_card *card) card->mmc_avail_type = avail_type; } +static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) +{ +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; + +/* +* Enhanced area feature support -- check whether the eMMC +* card has the Enhanced area enabled. If so, export enhanced +* area offset and size to user by adding sysfs interface. +*/ +if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && + (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +/* +* If the enhanced area is not enabled, disable these +* device attributes. +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ; + } +} + +static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) +{ +unsigned int part_size; +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +int idx; + +/* +* General purpose partition feature support -- +* If ext_csd has the size of general purpose partitions, +* set size, part_cfg, partition name in mmc_part. +*/ +if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & +EXT_CSD_PART_SUPPORT_PART_EN ) { +if (card->ext_csd.enhanced_area_en !=1 ) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; + } + +for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { +if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) +continue ; + part_size = + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] + <<16 ) + + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] + <<8 ) + + 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 ); + } + } +} +/* * Decode extended CSD. */
@@ -305,7 +392,6 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) {int err =0 , idx;unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;BUG_ON (!card);
@@ -402,80 +488,10 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { -/* -* Enhanced area feature support -- check whether the eMMC -* card has the Enhanced area enabled. If so, export enhanced -* area offset and size to user by adding sysfs interface. -*/ -if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && - (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +mmc_manage_enhanced_area (card, ext_csd); - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; - } +mmc_manage_gp_partitions (card, ext_csd); -/* -* General purpose partition feature support -- -* If ext_csd has the size of general purpose partitions, -* set size, part_cfg, partition name in mmc_part. -*/ -if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & -EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } - -for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { -if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) -continue ; - part_size = - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] - <<16 ) + - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] - <<8 ) + - 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 ); - } - } card->ext_csd.sec_trim_mult = ext_csd[EXT_CSD_SEC_TRIM_MULT ]; card->ext_csd.sec_erase_mult =
Signature · 1 line
1.7.9.5
v5 cover mmc: EXT_CSD_PARTITION_SETTING_COMPLETED bit not checked
In reply to Grégory SoutadéJEDEC standard requires that EXT_CSD_PARTITION_SETTING_COMPLETED bit must be set in order to take in account enhanced area and general purpose partitions (gp) values.
Current code doesn't checks this bit and blindly trust enhanced area and gp values. Moreover, "enhanced_area_en" attribute was set according to gp values but not necessary enhanced area one. It's then used to switch EXT_CSD_ERASE_GROUP_DEF bit which requires EXT_CSD_PARTITION_SETTING_COMPLETED. This attribute has been replaced by "partition_setting_completed" that match the expected behavior.
User is now warned in case of misconfiguration.
Plus, some code has been moved into functions for two reasons :
* functional reason (one behavior per function)
* Deep indentation result
Grégory Soutadé (3):
mmc: Move code that manages user area and gp partitions into functions
mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED before partitions computation
drivers/mmc/core/mmc.c | 171 +++++++++++++++++++++++++++------------------- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 + 3 files changed, 102 insertions(+), 73 deletions(-)
From commit 480cadc2b7e0fa2bbab20141efb547dfe0c3707c in master linux tree.
Changelog v4:
Second patch in v3 doesn't compile
Changelog v3:
Move code BEFORE fixing bugs.
Changelog v2:
Move code for user area and general purpose partitions into functions.
Signature · 1 line
1.7.9.5
Quoted 177 lines
Replace
ext_csd"enhanced_area_en" attribute by "partition_setting_completed".Quoted 176 lines
It was used whether or not enhanced user area is defined and without checks of
EXT_CSD_PARTITION_SETTING_COMPLETEDbit.Signed-off-by Grégory Soutadé <[email protected]>--- drivers/mmc/core/mmc.c | 8 +++++++- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-)Quoted 1 line
From commit
33caee39925b887a99a2400dc5c980097c3573f9in master linux tree.drivers/mmc/core/mmc.c +7
-1@@ -403,6 +403,12 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + }/* * Enhanced area feature support -- check whether the eMMC * card has the Enhanced area enabled. If so, export enhanced @@ -1309,7 +1315,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,include/linux/mmc/card.h +1
-1@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */ include/linux/mmc/mmc.h +2
-1@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0) --1.7.9.5
Quoted 101 lines · Ulf Hansson
Quoted 101 lines · Grégory Soutadé
Quoted 67 lines
Quoted 67 lines · Ulf Hansson
Quoted 48 lines
Quoted 48 lines · Grégory Soutadé
Quoted 46 lines
Quoted 46 lines · Ulf Hansson
Quoted 32 lines
Quoted 32 lines · Grégory Soutadé
Quoted 28 lines
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é <[email protected]>
---
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.
Quoted 8 lines
Quoted 8 lines
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
UffeHello,
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.
Quoted 6 lines
Quoted 6 lines
Fair enough.
Still I am wondering whether
hc_erase_grp_sz,hc_wp_grp_szandenhanced_area_enshould be updated ifEXT_CSD_PARTITION_SETTING_COMPLETEDisn'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 sizeand finally set
EXT_CSD_PARTITION_SETTING_COMPLETED.Thus these two parts must checks for setting completed before computing values.
Quoted 7 lines
Quoted 7 lines
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 setEXT_CSD_ERASE_GROUP_DEFand compute erase size again. I suggest using "partition_setting_completed" identifier which is common to the two functions and requiresEXT_CSD_ERASE_GROUP_DEFto be set.If you're ok with that, I'll submit another patch.
Seems reasonable. Please send a v2.
Kind regards
UffeQuoted 19 lines
Quoted 19 lines
Best regards
Grégory SoutadéQuoted 13 lines
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
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
PATCH v5 2/3 mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
In reply to Grégory SoutadéReplace ext_csd "enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks of EXT_CSD_PARTITION_SETTING_COMPLETED bit.
--- drivers/mmc/core/mmc.c | 14 +++++++++----- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 12 insertions(+), 6 deletions(-)
drivers/mmc/core/mmc.c +9 -5
@@ -314,7 +314,6 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd) hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ;/* * calculate the enhanced data area offset, in bytes */
@@ -355,13 +354,11 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { +if (card->ext_csd.partition_setting_completed) { hc_erase_grp_sz = ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; }for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {
@@ -488,6 +485,13 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + } +mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);
@@ -1324,7 +1328,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,
include/linux/mmc/card.h +1 -1
@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */
include/linux/mmc/mmc.h +2 -0
@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */
@@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
Signature · 1 line
1.7.9.5
Re: [PATCH v5 0002/0003] mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
Quoted 40 lines · Grégory Soutadé
Replace
ext_csd"enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks ofEXT_CSD_PARTITION_SETTING_COMPLETEDbit.Signed-off-by Grégory Soutadé <[email protected]>--- drivers/mmc/core/mmc.c | 14 +++++++++----- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 12 insertions(+), 6 deletions(-)drivers/mmc/core/mmc.c +1
-4@@ -314,7 +314,6 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd) hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ;/* * calculate the enhanced data area offset, in bytes */ @@ -355,13 +354,11 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { +if (card->ext_csd.partition_setting_completed) { hc_erase_grp_sz = ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; }for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {
drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 493
Quoted 9 lines
drivers/mmc/core/mmc.c +6
-0@@ -488,6 +485,13 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + }
I remembered that Ulf mentioned these brackets can be removed.
Best Regards,
Jaehoon Chung
Quoted 47 lines
drivers/mmc/core/mmc.c +2
-1+mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);@@ -1324,7 +1328,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,include/linux/mmc/card.h +1
-1@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */ include/linux/mmc/mmc.h +2
-0@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
PATCH v6 1/3 mmc: Move code that manages user area and gp partitions into functions
In reply to Grégory SoutadéMove code that manages user area and general purpose
partitions into functions.
--- drivers/mmc/core/mmc.c | 162 ++++++++++++++++++++++++++---------------------- 1 file changed, 89 insertions(+), 73 deletions(-)
drivers/mmc/core/mmc.c +89 -73
@@ -298,6 +298,93 @@ static void mmc_select_card_type(struct mmc_card *card) card->mmc_avail_type = avail_type; } +static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) +{ +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; + +/* +* Enhanced area feature support -- check whether the eMMC +* card has the Enhanced area enabled. If so, export enhanced +* area offset and size to user by adding sysfs interface. +*/ +if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && + (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +/* +* If the enhanced area is not enabled, disable these +* device attributes. +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ; + } +} + +static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) +{ +unsigned int part_size; +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +int idx; + +/* +* General purpose partition feature support -- +* If ext_csd has the size of general purpose partitions, +* set size, part_cfg, partition name in mmc_part. +*/ +if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & +EXT_CSD_PART_SUPPORT_PART_EN ) { +if (card->ext_csd.enhanced_area_en !=1 ) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; + } + +for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { +if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) +continue ; + part_size = + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] + <<16 ) + + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] + <<8 ) + + 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 ); + } + } +} +/* * Decode extended CSD. */
@@ -305,7 +392,6 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) {int err =0 , idx;unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;BUG_ON (!card);
@@ -402,80 +488,10 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { -/* -* Enhanced area feature support -- check whether the eMMC -* card has the Enhanced area enabled. If so, export enhanced -* area offset and size to user by adding sysfs interface. -*/ -if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && - (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +mmc_manage_enhanced_area (card, ext_csd); - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; - } +mmc_manage_gp_partitions (card, ext_csd); -/* -* General purpose partition feature support -- -* If ext_csd has the size of general purpose partitions, -* set size, part_cfg, partition name in mmc_part. -*/ -if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & -EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } - -for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { -if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) -continue ; - part_size = - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] - <<16 ) + - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] - <<8 ) + - 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 ); - } - } card->ext_csd.sec_trim_mult = ext_csd[EXT_CSD_SEC_TRIM_MULT ]; card->ext_csd.sec_erase_mult =
Signature · 1 line
1.7.9.5
PATCH v6 2/3 mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
In reply to Jaehoon ChungReplace ext_csd "enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks of EXT_CSD_PARTITION_SETTING_COMPLETED bit.
--- drivers/mmc/core/mmc.c | 13 ++++++++----- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 11 insertions(+), 6 deletions(-)
drivers/mmc/core/mmc.c +8 -5
@@ -314,7 +314,6 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd) hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ;/* * calculate the enhanced data area offset, in bytes */
@@ -355,13 +354,11 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { +if (card->ext_csd.partition_setting_completed) { hc_erase_grp_sz = ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; }for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {
@@ -488,6 +485,12 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) + card->ext_csd.partition_setting_completed =1 ; +else + card->ext_csd.partition_setting_completed =0 ; +mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);
@@ -1324,7 +1327,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,
include/linux/mmc/card.h +1 -1
@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */
include/linux/mmc/mmc.h +2 -0
@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */
@@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
Signature · 1 line
1.7.9.5
Quoted 104 lines · Jaehoon Chung
Quoted 40 lines · Grégory Soutadé
Replace
ext_csd"enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks ofEXT_CSD_PARTITION_SETTING_COMPLETEDbit.Signed-off-by Grégory Soutadé <[email protected]>--- drivers/mmc/core/mmc.c | 14 +++++++++----- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 12 insertions(+), 6 deletions(-)drivers/mmc/core/mmc.c +1
-4@@ -314,7 +314,6 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd) hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ;/* * calculate the enhanced data area offset, in bytes */ @@ -355,13 +354,11 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { +if (card->ext_csd.partition_setting_completed) { hc_erase_grp_sz = ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; }for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 493
Quoted 9 lines
drivers/mmc/core/mmc.c +6
-0@@ -488,6 +485,13 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + }I remembered that Ulf mentioned these brackets can be removed.
Best Regards,
Jaehoon ChungQuoted 47 lines
drivers/mmc/core/mmc.c +2
-1+mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);@@ -1324,7 +1328,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,include/linux/mmc/card.h +1
-1@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */ include/linux/mmc/mmc.h +2
-0@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
PATCH v6 3/3 mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED before partitions computation
In reply to Grégory SoutadéChecks EXT_CSD_PARTITION_SETTING_COMPLETED bit before
computing enhanced user area offset and size, and adding mmc general purpose partitions. The two needs EXT_CSD_PARTITION_SETTING_COMPLETED bit be set to be valid (as described in JEDEC standard).
Warn user in case of misconfiguration.
--- drivers/mmc/core/mmc.c | 81 ++++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 37 deletions(-)
drivers/mmc/core/mmc.c +44 -37
@@ -300,7 +300,13 @@ static void mmc_select_card_type(struct mmc_card *card)static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) { -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +u8 hc_erase_grp_sz, hc_wp_grp_sz; + +/* +* Disable these attributes by default +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ;/* * Enhanced area feature support -- check whether the eMMC
@@ -309,43 +315,41 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd)*/ if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +if (card->ext_csd.partition_setting_completed) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +pr_warn ("%s: defines enhanced area without partition setting complete\n" , +mmc_hostname (card->host)); + } } }static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) { -unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;int idx; +u8 hc_erase_grp_sz, hc_wp_grp_sz; +unsigned int part_size;/* * General purpose partition feature support --
@@ -354,18 +358,21 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.partition_setting_completed) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - } + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ];for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ])continue ; +if (card->ext_csd.partition_setting_completed ==0 ) { +pr_warn ("%s: has partition size defined without partition complete\n" , +mmc_hostname (card->host)); +break ; + } part_size = (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] << 16) +
Signature · 1 line
1.7.9.5
v6 cover mmc: EXT_CSD_PARTITION_SETTING_COMPLETED bit not checked
In reply to Grégory SoutadéJEDEC standard requires that EXT_CSD_PARTITION_SETTING_COMPLETED bit must be set in order to take in account enhanced area and general purpose partitions (gp) values.
Current code doesn't checks this bit and blindly trust enhanced area and gp values. Moreover, "enhanced_area_en" attribute was set according to gp values but not necessary enhanced area one. It's then used to switch EXT_CSD_ERASE_GROUP_DEF bit which requires EXT_CSD_PARTITION_SETTING_COMPLETED. This attribute has been replaced by "partition_setting_completed" that match the expected behavior.
User is now warned in case of misconfiguration.
Plus, some code has been moved into functions for two reasons :
* Functional reason (one behavior per function)
* Deep indentation result
Grégory Soutadé (3):
mmc: Move code that manages user area and gp partitions into functions
mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED before partitions computation
drivers/mmc/core/mmc.c | 172 ++++++++++++++++++++++++++-------------------- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 + 3 files changed, 102 insertions(+), 74 deletions(-)
From commit 7ec62d421bdf29cb31101ae2689f7f3a9906289a in master linux tree.
Changelog v5:
Remove some useless braces
Changelog v4:
Second patch in v3 doesn't compile
Changelog v3:
Move code BEFORE fixing bugs.
Changelog v2:
Move code for user area and general purpose partitions into functions.
Signature · 1 line
1.7.9.5
Ulf Hansson
AppliedRe: [PATCH v6 0001/0003] mmc: Move code that manages user area and gp partitions into functions
In reply to Grégory SoutadéQuoted 4 lines · Grégory Soutadé
Move code that manages user area and general purpose
partitions into functions.Signed-off-by Grégory Soutadé <[email protected]>
Thanks! Applied for next.
Kind regards
Uffe
Quoted 195 lines
--- drivers/mmc/core/mmc.c | 162 ++++++++++++++++++++++++++---------------------- 1 file changed, 89 insertions(+), 73 deletions(-)drivers/mmc/core/mmc.c +89
-74@@ -298,6 +298,93 @@ static void mmc_select_card_type(struct mmc_card *card) card->mmc_avail_type = avail_type; } +static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) +{ +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; + +/* +* Enhanced area feature support -- check whether the eMMC +* card has the Enhanced area enabled. If so, export enhanced +* area offset and size to user by adding sysfs interface. +*/ +if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && + (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +/* +* If the enhanced area is not enabled, disable these +* device attributes. +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ; + } +} + +static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) +{ +unsigned int part_size; +u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +int idx; + +/* +* General purpose partition feature support -- +* If ext_csd has the size of general purpose partitions, +* set size, part_cfg, partition name in mmc_part. +*/ +if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & +EXT_CSD_PART_SUPPORT_PART_EN ) { +if (card->ext_csd.enhanced_area_en !=1 ) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; + + card->ext_csd.enhanced_area_en =1 ; + } + +for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { +if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && + !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) +continue ; + part_size = + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] + <<16 ) + + (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] + <<8 ) + + 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 ); + } + } +} +/* * Decode extended CSD. */ @@ -305,7 +392,6 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) {int err =0 , idx;unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;BUG_ON (!card);@@ -402,80 +488,10 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { -/* -* Enhanced area feature support -- check whether the eMMC -* card has the Enhanced area enabled. If so, export enhanced -* area offset and size to user by adding sysfs interface. -*/ -if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && - (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +mmc_manage_enhanced_area (card, ext_csd); - card->ext_csd.enhanced_area_en =1 ; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; - } +mmc_manage_gp_partitions (card, ext_csd); -/* -* General purpose partition feature support -- -* If ext_csd has the size of general purpose partitions, -* set size, part_cfg, partition name in mmc_part. -*/ -if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] & -EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; - } - -for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) { -if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && - !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ]) -continue ; - part_size = - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] - <<16 ) + - (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] - <<8 ) + - 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 ); - } - } card->ext_csd.sec_trim_mult = ext_csd[EXT_CSD_SEC_TRIM_MULT ]; card->ext_csd.sec_erase_mult = --1.7.9.5
Ulf Hansson
AppliedRe: [PATCH v6 0002/0003] mmc: Replace "enhanced_area_en" attribute by "partition_setting_completed"
In reply to Grégory SoutadéQuoted 6 lines · Grégory Soutadé
Replace
ext_csd"enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks ofEXT_CSD_PARTITION_SETTING_COMPLETEDbit.Signed-off-by Grégory Soutadé <[email protected]>
Thanks! Applied for next.
Kind regards
Uffe
Quoted 196 lines
--- drivers/mmc/core/mmc.c | 13 ++++++++----- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 11 insertions(+), 6 deletions(-)drivers/mmc/core/mmc.c +8
-5@@ -314,7 +314,6 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd) hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ;/* * calculate the enhanced data area offset, in bytes */ @@ -355,13 +354,11 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { +if (card->ext_csd.partition_setting_completed) { hc_erase_grp_sz = ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; }for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {@@ -488,6 +485,12 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) + card->ext_csd.partition_setting_completed =1 ; +else + card->ext_csd.partition_setting_completed =0 ; +mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);@@ -1324,7 +1327,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,include/linux/mmc/card.h +1
-1@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */ include/linux/mmc/mmc.h +2
-1@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0) --1.7.9.5
Quoted 104 lines · Jaehoon Chung
Quoted 40 lines · Grégory Soutadé
Replace
ext_csd"enhanced_area_en" attribute by
"partition_setting_completed". It was used whether or not enhanced user area is defined and without checks ofEXT_CSD_PARTITION_SETTING_COMPLETEDbit.Signed-off-by Grégory Soutadé <[email protected]>--- drivers/mmc/core/mmc.c | 14 +++++++++----- include/linux/mmc/card.h | 2 +- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 12 insertions(+), 6 deletions(-)drivers/mmc/core/mmc.c +1
-4@@ -314,7 +314,6 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd) hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - card->ext_csd.enhanced_area_en =1 ;/* * calculate the enhanced data area offset, in bytes */ @@ -355,13 +354,11 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.enhanced_area_en !=1 ) { +if (card->ext_csd.partition_setting_completed) { hc_erase_grp_sz = ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; hc_wp_grp_sz = ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - - card->ext_csd.enhanced_area_en =1 ; }for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 493
Quoted 9 lines
drivers/mmc/core/mmc.c +6
-0@@ -488,6 +485,13 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_TRIM_MULT ]; card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT ];if (card->ext_csd.rev >=4 ) { +if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED ] & +EXT_CSD_PART_SETTING_COMPLETED ) { + card->ext_csd.partition_setting_completed =1 ; + }else { + card->ext_csd.partition_setting_completed =0 ; + }I remembered that Ulf mentioned these brackets can be removed.
Best Regards,
Jaehoon ChungQuoted 47 lines
drivers/mmc/core/mmc.c +2
-1+mmc_manage_enhanced_area (card, ext_csd);mmc_manage_gp_partitions (card, ext_csd);@@ -1324,7 +1328,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,* If enhanced_area_en is TRUE, host needs to enable ERASE_GRP_DEF * bit. This bit will be lost every time after a reset or power off. */ -if (card->ext_csd.enhanced_area_en || +if (card->ext_csd.partition_setting_completed || (card->ext_csd.rev >=3 && (host->caps2 &MMC_CAP2_HC_ERASE_SZ ))) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_ERASE_GROUP_DEF, 1,include/linux/mmc/card.h +1
-1@@ -74,7 +74,7 @@ struct mmc_ext_csd {unsigned int sec_trim_mult;/* Secure trim multiplier */ unsigned int sec_erase_mult;/* Secure erase multiplier */ unsigned int trim_timeout;/* In milliseconds */ -bool enhanced_area_en;/* enable bit */ +bool partition_setting_completed;/* enable bit */ unsigned long long enhanced_area_offset;/* Units: Byte */ unsigned int enhanced_area_size;/* Units: KB */ unsigned int cache_size;/* Units: KB */ include/linux/mmc/mmc.h +2
-0@@ -281,6 +281,7 @@ struct _mmc_csd {#define EXT_CSD_EXP_EVENTS_CTRL 56/* R/W, 2 bytes */ #define EXT_CSD_DATA_SECTOR_SIZE 61/* R */ #define EXT_CSD_GP_SIZE_MULT 143/* R/W */ +#define EXT_CSD_PARTITION_SETTING_COMPLETED 155/* R/W */ #define EXT_CSD_PARTITION_ATTRIBUTE 156/* R/W */ #define EXT_CSD_PARTITION_SUPPORT 160/* RO */ #define EXT_CSD_HPI_MGMT 161/* R/W */ @@ -349,6 +350,7 @@ struct _mmc_csd {#define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3)#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4) +#define EXT_CSD_PART_SETTING_COMPLETED (0x1)#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)#define EXT_CSD_CMD_SET_NORMAL (1<<0)
Ulf Hansson
AppliedRe: [PATCH v6 0003/0003] mmc: Checks EXT_CSD_PARTITION_SETTING_COMPLETED before partitions computation
In reply to Grégory SoutadéQuoted 8 lines · Grégory Soutadé
Checks
EXT_CSD_PARTITION_SETTING_COMPLETEDbit before
computing enhanced user area offset and size, and adding mmc general purpose partitions. The two needsEXT_CSD_PARTITION_SETTING_COMPLETEDbit be set to be valid (as described in JEDEC standard).
Warn user in case of misconfiguration.Signed-off-by Grégory Soutadé <[email protected]>
Thanks! Applied for next.
Kind regards
Uffe
Quoted 125 lines
--- drivers/mmc/core/mmc.c | 81 ++++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 37 deletions(-)drivers/mmc/core/mmc.c +44
-38@@ -300,7 +300,13 @@ static void mmc_select_card_type(struct mmc_card *card)static void mmc_manage_enhanced_area (struct mmc_card *card,u8 *ext_csd) { -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ; +u8 hc_erase_grp_sz, hc_wp_grp_sz; + +/* +* Disable these attributes by default +*/ + card->ext_csd.enhanced_area_offset = -EINVAL ; + card->ext_csd.enhanced_area_size = -EINVAL ;/* * Enhanced area feature support -- check whether the eMMC @@ -309,43 +315,41 @@ static void mmc_manage_enhanced_area(struct mmc_card *card, u8 *ext_csd)*/ if ((ext_csd[EXT_CSD_PARTITION_SUPPORT ] &0x2 ) && (ext_csd[EXT_CSD_PARTITION_ATTRIBUTE ] &0x1 )) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; +if (card->ext_csd.partition_setting_completed) { + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; -/* -* calculate the enhanced data area offset, in bytes -*/ - card->ext_csd.enhanced_area_offset = - (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + - (ext_csd[137 ] <<8 ) + ext_csd[136 ]; -if (mmc_card_blockaddr (card)) - card->ext_csd.enhanced_area_offset <<=9 ; -/* -* calculate the enhanced data area size, in kilobytes -*/ - card->ext_csd.enhanced_area_size = - (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + - ext_csd[140 ]; - card->ext_csd.enhanced_area_size *= - (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); - card->ext_csd.enhanced_area_size <<=9 ; - }else { -/* -* If the enhanced area is not enabled, disable these -* device attributes. -*/ - card->ext_csd.enhanced_area_offset = -EINVAL ; - card->ext_csd.enhanced_area_size = -EINVAL ; +/* +* calculate the enhanced data area offset, in bytes +*/ + card->ext_csd.enhanced_area_offset = + (ext_csd[139 ] <<24 ) + (ext_csd[138 ] <<16 ) + + (ext_csd[137 ] <<8 ) + ext_csd[136 ]; +if (mmc_card_blockaddr (card)) + card->ext_csd.enhanced_area_offset <<=9 ; +/* +* calculate the enhanced data area size, in kilobytes +*/ + card->ext_csd.enhanced_area_size = + (ext_csd[142 ] <<16 ) + (ext_csd[141 ] <<8 ) + + ext_csd[140 ]; + card->ext_csd.enhanced_area_size *= + (size_t )(hc_erase_grp_sz * hc_wp_grp_sz); + card->ext_csd.enhanced_area_size <<=9 ; + }else { +pr_warn ("%s: defines enhanced area without partition setting complete\n" , +mmc_hostname (card->host)); + } } }static void mmc_manage_gp_partitions (struct mmc_card *card,u8 *ext_csd) { -unsigned int part_size; -u8 hc_erase_grp_sz =0 , hc_wp_grp_sz =0 ;int idx; +u8 hc_erase_grp_sz, hc_wp_grp_sz; +unsigned int part_size;/* * General purpose partition feature support -- @@ -354,18 +358,21 @@ static void mmc_manage_gp_partitions(struct mmc_card *card, u8 *ext_csd)*/ if (ext_csd[EXT_CSD_PARTITION_SUPPORT ] &EXT_CSD_PART_SUPPORT_PART_EN ) { -if (card->ext_csd.partition_setting_completed) { - hc_erase_grp_sz = - ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; - hc_wp_grp_sz = - ext_csd[EXT_CSD_HC_WP_GRP_SIZE ]; - } + hc_erase_grp_sz = + ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE ]; + hc_wp_grp_sz = + ext_csd[EXT_CSD_HC_WP_GRP_SIZE ];for (idx =0 ; idx <MMC_NUM_GP_PARTITION ; idx++) {if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +1 ] && !ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ])continue ; +if (card->ext_csd.partition_setting_completed ==0 ) { +pr_warn ("%s: has partition size defined without partition complete\n" , +mmc_hostname (card->host)); +break ; + } part_size = (ext_csd[EXT_CSD_GP_SIZE_MULT + idx *3 +2 ] << 16) + --1.7.9.5