Hi Simon,
On Sat, 28 Jan 2023 at 23:01, Simon Glass <s...@chromium.org> wrote:
>
> Hi Loic,
>
> On Thu, 26 Jan 2023 at 02:24, Loic Poulain <loic.poul...@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.poul...@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 <s...@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
