Re: [External] : RE: [PATCH RESEND v3] mmc-utils: Add eMMC erase command support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

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@xxxxxxxxxx>
Signed-off-by: Bean Huo <beanhuo@xxxxxxxxxx>
Signed-off-by: Kimito Sakata <kimito.sakata@xxxxxxxxxx>
Reviewed-by: Kenneth Gibbons <kenny.gibbons@xxxxxxxxxx>

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)




[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux