[PATCH v2 2/3] mmc: erase: Use TRIM erase when available

[Loic Poulain]

Hi Simon,

On Sat, 28 Jan 2023 at 23:01, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Loic,
>
> On Thu, 26 Jan 2023 at 02:24, Loic Poulain <loic.poulain at linaro.org> wrote:
> >
> > The default erase command applies on erase group unit, and
> > simply round down to erase group size. When the start block
> > is not aligned to erase group size (e.g. erasing partition)
> > it causes unwanted erasing of the previous blocks, part of
> > the same erase group (e.g. owned by other logical partition,
> > or by the partition table itself).
> >
> > To prevent this issue, a simple solution is to use TRIM as
> > argument of the Erase command, which is usually supported
> > with eMMC > 4.0, and allow to apply erase operation to write
> > blocks instead of erase group
> >
> > Signed-off-by: Loic Poulain <loic.poulain at linaro.org>
> > ---
> > v2: Add mmc unit test change to the series
> >
> >  drivers/mmc/mmc_write.c | 34 +++++++++++++++++++++++-----------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Please see below
>
> >
> > diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
> > index 5b7aeeb012..a6f93380dd 100644
> > --- a/drivers/mmc/mmc_write.c
> > +++ b/drivers/mmc/mmc_write.c
> > @@ -15,7 +15,7 @@
> >  #include <linux/math64.h>
> >  #include "mmc_private.h"
> >
> > -static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> > +static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt, u32 args)
> >  {
> >         struct mmc_cmd cmd;
> >         ulong end;
> > @@ -52,7 +52,7 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> >                 goto err_out;
> >
> >         cmd.cmdidx = MMC_CMD_ERASE;
> > -       cmd.cmdarg = MMC_ERASE_ARG;
> > +       cmd.cmdarg = args ? args : MMC_ERASE_ARG;
> >         cmd.resp_type = MMC_RSP_R1b;
> >
> >         err = mmc_send_cmd(mmc, &cmd, NULL);
> > @@ -77,7 +77,7 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt)
> >  #endif
> >         int dev_num = block_dev->devnum;
> >         int err = 0;
> > -       u32 start_rem, blkcnt_rem;
> > +       u32 start_rem, blkcnt_rem, erase_args = 0;
> >         struct mmc *mmc = find_mmc_device(dev_num);
> >         lbaint_t blk = 0, blk_r = 0;
> >         int timeout_ms = 1000;
> > @@ -97,13 +97,25 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt)
> >          */
> >         err = div_u64_rem(start, mmc->erase_grp_size, &start_rem);
> >         err = div_u64_rem(blkcnt, mmc->erase_grp_size, &blkcnt_rem);
> > -       if (start_rem || blkcnt_rem)
> > -               printf("\n\nCaution! Your devices Erase group is 0x%x\n"
> > -                      "The erase range would be change to "
> > -                      "0x" LBAF "~0x" LBAF "\n\n",
> > -                      mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 1),
> > -                      ((start + blkcnt + mmc->erase_grp_size - 1)
> > -                      & ~(mmc->erase_grp_size - 1)) - 1);
> > +       if (start_rem || blkcnt_rem) {
> > +               if (mmc->can_trim) {
> > +                       /* Trim function applies the erase operation to write
> > +                        * blocks instead of erase groups.
> > +                        */
> > +                       erase_args = MMC_TRIM_ARG;
> > +               } else {
> > +                       /* The card ignores all LSB's below the erase group
> > +                        * size, rounding down the addess to a erase group
> > +                        * boundary.
> > +                        */
> > +                       printf("\n\nCaution! Your devices Erase group is 0x%x\n"
> > +                              "The erase range would be change to "
> > +                              "0x" LBAF "~0x" LBAF "\n\n",
> > +                              mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 1),
> > +                              ((start + blkcnt + mmc->erase_grp_size - 1)
> > +                              & ~(mmc->erase_grp_size - 1)) - 1);
>
> Should this return an error, or just go ahead?

It would indeed make sense to return an error since mmc_erase does not
perform what we expect. Now, since this behavior exists for a while,
we may also want to keep it for legacy, though it should be a corner
case...

Regards,
Loic