2021-03-24 16:49:28

by Bean Huo

[permalink] [raw]
Subject: [PATCH v2] mmc-utils: Add eMMC erase command support

From: Kimito Sakata <[email protected]>

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.

Co-developed-by: Bean Huo <[email protected]>
Signed-off-by: Bean Huo <[email protected]>
Reviewed-by: Kenneth Gibbons <[email protected]>
---

Hi Ulf,
Please help us review this mmc-utils patch, and if agree, it is
possible to make it merged in the official mmc-utils.

Changelog:

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 | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++
mmc_cmds.h | 1 +
4 files changed, 156 insertions(+), 1 deletion(-)

diff --git a/mmc.c b/mmc.c
index 50c9c9e..cb29a65 100644
--- a/mmc.c
+++ b/mmc.c
@@ -215,6 +215,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 648fb26..90b7fb5 100644
--- a/mmc.h
+++ b/mmc.h
@@ -34,7 +34,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
*/
@@ -61,6 +69,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
@@ -177,6 +186,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 fb37189..17986e3 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -2435,6 +2435,141 @@ 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)
+{
+#ifndef MMC_IOC_MULTI_CMD
+ fprintf(stderr, "mmc-utils has been compiled without MMC_IOC_MULTI_CMD"
+ " support, needed by erase.\n");
+ return -ENOTSUP;
+#else
+ 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;
+ }
+
+ 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);
+
+ if (end < start) {
+ fprintf(stderr, "erase start [0x%08x] > erase end [0x%08x]\n",
+ start, end);
+ exit(1);
+ }
+
+ 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);
+ }
+
+ dev_fd = open(argv[4], O_RDWR);
+ if (dev_fd < 0) {
+ perror(argv[4]);
+ exit(1);
+ }
+
+ 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;
+ }
+
+ }
+ 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.25.1


2021-03-30 12:44:29

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH v2] mmc-utils: Add eMMC erase command support

On Wed, 24 Mar 2021 at 17:45, Bean Huo <[email protected]> wrote:
>
> From: Kimito Sakata <[email protected]>
>
> 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.
>
> Co-developed-by: Bean Huo <[email protected]>
> Signed-off-by: Bean Huo <[email protected]>
> Reviewed-by: Kenneth Gibbons <[email protected]>
> ---
>
> Hi Ulf,
> Please help us review this mmc-utils patch, and if agree, it is
> possible to make it merged in the official mmc-utils.
>
> Changelog:
>
> 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 | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> mmc_cmds.h | 1 +
> 4 files changed, 156 insertions(+), 1 deletion(-)
>
> diff --git a/mmc.c b/mmc.c
> index 50c9c9e..cb29a65 100644
> --- a/mmc.c
> +++ b/mmc.c
> @@ -215,6 +215,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 648fb26..90b7fb5 100644
> --- a/mmc.h
> +++ b/mmc.h
> @@ -34,7 +34,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
> */
> @@ -61,6 +69,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
> @@ -177,6 +186,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 fb37189..17986e3 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2435,6 +2435,141 @@ 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)
> +{
> +#ifndef MMC_IOC_MULTI_CMD

In kernel v4.4 we added the multi cmd support, which is quite some
time ago. So, I think it's time to drop these ifdef hackary from the
userland tool. At least, we shouldn't need it for new kinds of
features that we add.

> + fprintf(stderr, "mmc-utils has been compiled without MMC_IOC_MULTI_CMD"
> + " support, needed by erase.\n");
> + return -ENOTSUP;
> +#else
> + int ret = 0;
> + struct mmc_ioc_multi_cmd *multi_cmd;
> +

[...]

Kind regards
Uffe

2021-03-30 21:38:27

by Kimito Sakata

[permalink] [raw]
Subject: Re: [External] : Re: [PATCH v2] mmc-utils: Add eMMC erase command support



On 3/30/2021 6:39 AM, Ulf Hansson wrote:
> On Wed, 24 Mar 2021 at 17:45, Bean Huo <[email protected]> wrote:
>> From: Kimito Sakata <[email protected]>
>>
>> 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.
>>
>> Co-developed-by: Bean Huo <[email protected]>
>> Signed-off-by: Bean Huo <[email protected]>
>> Reviewed-by: Kenneth Gibbons <[email protected]>
>> ---
>>
>> Hi Ulf,
>> Please help us review this mmc-utils patch, and if agree, it is
>> possible to make it merged in the official mmc-utils.
>>
>> Changelog:
>>
>> 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 | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> mmc_cmds.h | 1 +
>> 4 files changed, 156 insertions(+), 1 deletion(-)
>>
>> diff --git a/mmc.c b/mmc.c
>> index 50c9c9e..cb29a65 100644
>> --- a/mmc.c
>> +++ b/mmc.c
>> @@ -215,6 +215,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 648fb26..90b7fb5 100644
>> --- a/mmc.h
>> +++ b/mmc.h
>> @@ -34,7 +34,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
>> */
>> @@ -61,6 +69,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
>> @@ -177,6 +186,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 fb37189..17986e3 100644
>> --- a/mmc_cmds.c
>> +++ b/mmc_cmds.c
>> @@ -2435,6 +2435,141 @@ 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)
>> +{
>> +#ifndef MMC_IOC_MULTI_CMD
> In kernel v4.4 we added the multi cmd support, which is quite some
> time ago. So, I think it's time to drop these ifdef hackary from the
> userland tool. At least, we shouldn't need it for new kinds of
> features that we add.
Ulf
Do you want us to take out theĀ  MMC_IOC_MULTI_CMD ifdef and resubmit?
Kimito

>
>> + fprintf(stderr, "mmc-utils has been compiled without MMC_IOC_MULTI_CMD"
>> + " support, needed by erase.\n");
>> + return -ENOTSUP;
>> +#else
>> + int ret = 0;
>> + struct mmc_ioc_multi_cmd *multi_cmd;
>> +
> [...]
>
> Kind regards
> Uffe

2021-03-31 09:32:32

by Ulf Hansson

[permalink] [raw]
Subject: Re: [External] : Re: [PATCH v2] mmc-utils: Add eMMC erase command support

On Tue, 30 Mar 2021 at 23:36, <[email protected]> wrote:
>
>
>
> On 3/30/2021 6:39 AM, Ulf Hansson wrote:
> > On Wed, 24 Mar 2021 at 17:45, Bean Huo <[email protected]> wrote:
> >> From: Kimito Sakata <[email protected]>
> >>
> >> 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.
> >>
> >> Co-developed-by: Bean Huo <[email protected]>
> >> Signed-off-by: Bean Huo <[email protected]>
> >> Reviewed-by: Kenneth Gibbons <[email protected]>
> >> ---
> >>
> >> Hi Ulf,
> >> Please help us review this mmc-utils patch, and if agree, it is
> >> possible to make it merged in the official mmc-utils.
> >>
> >> Changelog:
> >>
> >> 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 | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> mmc_cmds.h | 1 +
> >> 4 files changed, 156 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mmc.c b/mmc.c
> >> index 50c9c9e..cb29a65 100644
> >> --- a/mmc.c
> >> +++ b/mmc.c
> >> @@ -215,6 +215,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 648fb26..90b7fb5 100644
> >> --- a/mmc.h
> >> +++ b/mmc.h
> >> @@ -34,7 +34,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
> >> */
> >> @@ -61,6 +69,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
> >> @@ -177,6 +186,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 fb37189..17986e3 100644
> >> --- a/mmc_cmds.c
> >> +++ b/mmc_cmds.c
> >> @@ -2435,6 +2435,141 @@ 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)
> >> +{
> >> +#ifndef MMC_IOC_MULTI_CMD
> > In kernel v4.4 we added the multi cmd support, which is quite some
> > time ago. So, I think it's time to drop these ifdef hackary from the
> > userland tool. At least, we shouldn't need it for new kinds of
> > features that we add.
> Ulf
> Do you want us to take out the MMC_IOC_MULTI_CMD ifdef and resubmit?

Yes, please.

Moreover, we should probably also remove all the other #ifndef
MMC_IOC_MULTI_CMD hacks that we currently have in the code. But that's
a separate patch.

> Kimito
>
> >
> >> + fprintf(stderr, "mmc-utils has been compiled without MMC_IOC_MULTI_CMD"
> >> + " support, needed by erase.\n");
> >> + return -ENOTSUP;
> >> +#else
> >> + int ret = 0;
> >> + struct mmc_ioc_multi_cmd *multi_cmd;
> >> +
> > [...]

Kind regards
Uffe

2021-03-31 14:33:08

by Kimito Sakata

[permalink] [raw]
Subject: Re: [External] : Re: [PATCH v2] mmc-utils: Add eMMC erase command support

OK Ulf. I'll make the changes and resubmit.
Thanks
Kimito

On 3/31/2021 3:30 AM, Ulf Hansson wrote:
> On Tue, 30 Mar 2021 at 23:36, <[email protected]> wrote:
>>
>>
>> On 3/30/2021 6:39 AM, Ulf Hansson wrote:
>>> On Wed, 24 Mar 2021 at 17:45, Bean Huo <[email protected]> wrote:
>>>> From: Kimito Sakata <[email protected]>
>>>>
>>>> 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.
>>>>
>>>> Co-developed-by: Bean Huo <[email protected]>
>>>> Signed-off-by: Bean Huo <[email protected]>
>>>> Reviewed-by: Kenneth Gibbons <[email protected]>
>>>> ---
>>>>
>>>> Hi Ulf,
>>>> Please help us review this mmc-utils patch, and if agree, it is
>>>> possible to make it merged in the official mmc-utils.
>>>>
>>>> Changelog:
>>>>
>>>> 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 | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> mmc_cmds.h | 1 +
>>>> 4 files changed, 156 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mmc.c b/mmc.c
>>>> index 50c9c9e..cb29a65 100644
>>>> --- a/mmc.c
>>>> +++ b/mmc.c
>>>> @@ -215,6 +215,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 648fb26..90b7fb5 100644
>>>> --- a/mmc.h
>>>> +++ b/mmc.h
>>>> @@ -34,7 +34,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
>>>> */
>>>> @@ -61,6 +69,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
>>>> @@ -177,6 +186,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 fb37189..17986e3 100644
>>>> --- a/mmc_cmds.c
>>>> +++ b/mmc_cmds.c
>>>> @@ -2435,6 +2435,141 @@ 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)
>>>> +{
>>>> +#ifndef MMC_IOC_MULTI_CMD
>>> In kernel v4.4 we added the multi cmd support, which is quite some
>>> time ago. So, I think it's time to drop these ifdef hackary from the
>>> userland tool. At least, we shouldn't need it for new kinds of
>>> features that we add.
>> Ulf
>> Do you want us to take out the MMC_IOC_MULTI_CMD ifdef and resubmit?
> Yes, please.
>
> Moreover, we should probably also remove all the other #ifndef
> MMC_IOC_MULTI_CMD hacks that we currently have in the code. But that's
> a separate patch.
>
>> Kimito
>>
>>>> + fprintf(stderr, "mmc-utils has been compiled without MMC_IOC_MULTI_CMD"
>>>> + " support, needed by erase.\n");
>>>> + return -ENOTSUP;
>>>> +#else
>>>> + int ret = 0;
>>>> + struct mmc_ioc_multi_cmd *multi_cmd;
>>>> +
>>> [...]
> Kind regards
> Uffe