Messages in this thread | | | Subject | Re: [External] : RE: [PATCH RESEND v3] mmc-utils: Add eMMC erase command support | From | kimito.sakata@oracle ... | Date | Wed, 28 Apr 2021 09:59:13 -0600 |
| |
Avri
Thanks. I will respond to you when I get the chance to go through all of it.
Kimito
On 4/27/2021 11:57 PM, Avri Altman wrote: > Hi Kimito & Bean, > >> From: Kimito Sakata <kimito.sakata@oracle.com> >> >> we have been using this erase feature for a while, but it is >> still not merged into the upstream mmc-utils. Especially, for >> the customer, every time when they update the mmc-utils, they >> should re-install this patch again, let's try to make this >> erase command upstreamed in the mmc-utils. >> >> We need to send 3 MMC commands and it is important that they >> stay in sequence. Therefore we are using MMC_IOC_MULTI_CMD. > You might also want to indicate on which platform it was tested. > >> Co-developed-by: Bean Huo <beanhuo@micron.com> >> Signed-off-by: Bean Huo <beanhuo@micron.com> >> Signed-off-by: Kimito Sakata <kimito.sakata@oracle.com> >> Reviewed-by: Kenneth Gibbons <kenny.gibbons@oracle.com> >> >> Changelog: >> v2--v3: >> 1. Remove redundant ifndef >> >> V1--V2: >> 1. refactor Kimito's original patch >> 2. change to use MMC_IOC_MULTI_CMD >> 3. add checkup if eMMC devie supports secure erase/trim >> --- >> mmc.c | 8 ++++ >> mmc.h | 13 +++++- >> mmc_cmds.c | 130 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> mmc_cmds.h | 1 + >> 4 files changed, 151 insertions(+), 1 deletion(-) >> >> diff --git a/mmc.c b/mmc.c >> index f3d724b..eb2638b 100644 >> --- a/mmc.c >> +++ b/mmc.c >> @@ -229,6 +229,14 @@ static struct Command commands[] = { >> "Run Field Firmware Update with <image name> on <device>.\n", >> NULL >> }, >> + { do_erase, -4, >> + "erase", "<type> " "<start address> " "<end address> " "<device>\n" >> + "Send Erase CMD38 with specific argument to the <device>\n\n" >> + "NOTE!: This will delete all user data in the specified region of the >> device\n" >> + "<type> must be: legacy | discard | secure-erase | " >> + "secure-trim1 | secure-trim2 | trim \n", >> + NULL >> + }, >> { 0, 0, 0, 0 } >> }; >> >> diff --git a/mmc.h b/mmc.h >> index 5754a9d..e9766d7 100644 >> --- a/mmc.h >> +++ b/mmc.h >> @@ -35,7 +35,15 @@ >> #define MMC_SET_WRITE_PROT 28 /* ac [31:0] data addr R1b */ >> #define MMC_CLEAR_WRITE_PROT 29 /* ac [31:0] data addr R1b */ >> #define MMC_SEND_WRITE_PROT_TYPE 31 /* ac [31:0] data addr R1 */ >> - >> +#define MMC_ERASE_GROUP_START 35 /* ac [31:0] data addr R1 */ >> +#define MMC_ERASE_GROUP_END 36 /* ac [31:0] data addr R1 */ >> +#define MMC_ERASE 38 /* ac [31] Secure request >> + [30:16] set to 0 >> + [15] Force Garbage Collect request >> + [14:2] set to 0 >> + [1] Discard Enable >> + [0] Identify Write Blocks for >> + Erase (or TRIM Enable) R1b */ >> /* >> * EXT_CSD fields >> */ >> @@ -62,6 +70,7 @@ >> #define EXT_CSD_CACHE_SIZE_2 251 >> #define EXT_CSD_CACHE_SIZE_1 250 >> #define EXT_CSD_CACHE_SIZE_0 249 >> +#define EXT_CSD_SEC_FEATURE_SUPPORT 231 >> #define EXT_CSD_BOOT_INFO 228 /* R/W */ >> #define EXT_CSD_HC_ERASE_GRP_SIZE 224 >> #define EXT_CSD_HC_WP_GRP_SIZE 221 >> @@ -190,6 +199,8 @@ >> #define EXT_CSD_REV_V4_2 2 >> #define EXT_CSD_REV_V4_1 1 >> #define EXT_CSD_REV_V4_0 0 >> +#define EXT_CSD_SEC_GB_CL_EN (1<<4) >> +#define EXT_CSD_SEC_ER_EN (1<<0) >> >> >> /* From kernel linux/mmc/core.h */ >> diff --git a/mmc_cmds.c b/mmc_cmds.c >> index 6c24cea..24f80db 100644 >> --- a/mmc_cmds.c >> +++ b/mmc_cmds.c >> @@ -2514,6 +2514,136 @@ int do_cache_dis(int nargs, char **argv) >> return do_cache_ctrl(0, nargs, argv); >> } >> >> +static int erase(int dev_fd, __u32 argin, __u32 start, __u32 end) >> +{ >> + int ret = 0; >> + struct mmc_ioc_multi_cmd *multi_cmd; >> + >> + multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + >> + 3 * sizeof(struct mmc_ioc_cmd)); >> + if (!multi_cmd) { >> + perror("Failed to allocate memory"); >> + return -ENOMEM; >> + } > I am missing reference to ERASE_GROUP_DEF. > It affects the timeout, size of erase group etc. > Don't you need to query for before starting to build the command? > >> + >> + multi_cmd->num_of_cmds = 3; >> + /* Set erase start address */ >> + multi_cmd->cmds[0].opcode = MMC_ERASE_GROUP_START; >> + multi_cmd->cmds[0].arg = start; >> + multi_cmd->cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | >> MMC_CMD_AC; >> + multi_cmd->cmds[0].write_flag = 1; >> + >> + /* Set erase end address */ >> + multi_cmd->cmds[1].opcode = MMC_ERASE_GROUP_END; >> + multi_cmd->cmds[1].arg = end; >> + multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | >> MMC_CMD_AC; >> + multi_cmd->cmds[1].write_flag = 1; >> + >> + /* Send Erase Command */ >> + multi_cmd->cmds[2].opcode = MMC_ERASE; >> + multi_cmd->cmds[2].arg = argin; >> + multi_cmd->cmds[2].cmd_timeout_ms = 300*255*255; >> + multi_cmd->cmds[2].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | >> MMC_CMD_AC; >> + multi_cmd->cmds[2].write_flag = 1; >> + >> + /* send erase cmd with multi-cmd */ >> + ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); >> + if (ret) >> + perror("Erase multi-cmd ioctl"); >> + >> + free(multi_cmd); >> + return ret; >> +#endif >> +} >> + >> +int do_erase(int nargs, char **argv) >> +{ >> + int dev_fd, ret; >> + char *print_str; >> + char **eptr = NULL; >> + __u8 ext_csd[512], checkup_mask = 0; >> + __u32 arg, start, end; >> + >> + if (nargs != 5) { >> + fprintf(stderr, "Usage: erase <type> <start addr> <end addr> >> </path/to/mmcblkX>\n"); >> + exit(1); >> + } >> + >> + if (strstr(argv[2], "0x") || strstr(argv[2], "0X")) >> + start = strtol(argv[2], eptr, 16); >> + else >> + start = strtol(argv[2], eptr, 10); >> + >> + if (strstr(argv[3], "0x") || strstr(argv[3], "0X")) >> + end = strtol(argv[3], eptr, 16); >> + else >> + end = strtol(argv[3], eptr, 10); > You are not really using eptr - maybe just call strtol with NULL? > >> + >> + if (end < start) { >> + fprintf(stderr, "erase start [0x%08x] > erase end [0x%08x]\n", >> + start, end); >> + exit(1); >> + } > Start & End are in bytes for media < 2GB, and in sectors for media >=2GB. > Should also check for legal erase group restrictions. > >> + >> + if (strcmp(argv[1], "legacy") == 0) { >> + arg = 0x00000000; >> + print_str = "Legacy Erase"; >> + } else if (strcmp(argv[1], "discard") == 0) { >> + arg = 0x00000003; >> + print_str = "Discard"; >> + } else if (strcmp(argv[1], "secure-erase") == 0) { >> + print_str = "Secure Erase"; >> + checkup_mask = EXT_CSD_SEC_ER_EN; >> + arg = 0x80000000; >> + } else if (strcmp(argv[1], "secure-trim1") == 0) { >> + print_str = "Secure Trim Step 1"; >> + checkup_mask = EXT_CSD_SEC_ER_EN | EXT_CSD_SEC_GB_CL_EN; >> + arg = 0x80000001; >> + } else if (strcmp(argv[1], "secure-trim2") == 0) { >> + print_str = "Secure Trim Step 2"; >> + checkup_mask = EXT_CSD_SEC_ER_EN | EXT_CSD_SEC_GB_CL_EN; >> + arg = 0x80008000; >> + } else if (strcmp(argv[1], "trim") == 0) { >> + print_str = "Trim"; >> + checkup_mask = EXT_CSD_SEC_GB_CL_EN; >> + arg = 0x00000001; >> + } else { >> + fprintf(stderr, "Unknown erase type: %s\n", argv[1]); >> + exit(1); >> + } > Maybe use an enum for the erase type to avoid the strcmp? > Then need to update the usage message. > >> + >> + dev_fd = open(argv[4], O_RDWR); >> + if (dev_fd < 0) { >> + perror(argv[4]); >> + exit(1); >> + } > How do you differentiate between mmc and SD devices? > And for SD you should send different commands(35/36 vs. 32/33). > Do you intend to support SD devices as well? In another patch? > >> + >> + if (checkup_mask) { >> + ret = read_extcsd(dev_fd, ext_csd); >> + if (ret) { >> + fprintf(stderr, "Could not read EXT_CSD from %s\n", >> + argv[4]); >> + goto out; >> + } >> + if ((checkup_mask & ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT]) != >> + checkup_mask) { >> + fprintf(stderr, "%s is not supported in %s\n", >> + print_str, argv[4]); >> + ret = -ENOTSUP; >> + goto out; >> + } > EXT_CSD_SEC_FEATURE_SUPPORT might also contains the sanitize bit, > as well as reserved bits that you shouldn't assume they are 0, so I am not sure that this condition is correct. > The checkup only refers to bit 0 and bit 4 - maybe add a helper that accept &args and returns 0 if allowed... > > > Thanks, > Avri > >> + >> + } >> + printf("Executing %s from 0x%08x to 0x%08x\n", print_str, start, end); >> + >> + ret = erase(dev_fd, arg, start, end); >> +out: >> + printf(" %s %s!\n\n", print_str, ret ? "Failed" : "Succeed"); >> + close(dev_fd); >> + return ret; >> +} >> + >> + >> int do_ffu(int nargs, char **argv) >> { >> #ifndef MMC_IOC_MULTI_CMD >> diff --git a/mmc_cmds.h b/mmc_cmds.h >> index 9d3246c..8331ab2 100644 >> --- a/mmc_cmds.h >> +++ b/mmc_cmds.h >> @@ -45,3 +45,4 @@ int do_ffu(int nargs, char **argv); >> int do_read_scr(int argc, char **argv); >> int do_read_cid(int argc, char **argv); >> int do_read_csd(int argc, char **argv); >> +int do_erase(int nargs, char **argv); >> -- >> 2.24.1 (Apple Git-126)
| |