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

[kimito.sakata@oracle.com]

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, <kimito.sakata@oracle.com> wrote:
>>
>>
>> On 3/30/2021 6:39 AM, Ulf Hansson wrote:
>>> On Wed, 24 Mar 2021 at 17:45, Bean Huo <huobean@gmail.com> wrote:
>>>> 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.
>>>>
>>>> Co-developed-by: Bean Huo <beanhuo@micron.com>
>>>> Signed-off-by: Bean Huo <beanhuo@micron.com>
>>>> Reviewed-by: Kenneth Gibbons <kenny.gibbons@oracle.com>
>>>> ---
>>>>
>>>> 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