Re: [PATCH] mmc: core: Fix error propagation for some ioctl commands

[Ulf Hansson]

On Thu, 17 Aug 2023 at 12:03, Avri Altman <Avri.Altman@xxxxxxx> wrote:
>
> > Userspace has currently has no way of checking the internal R1 response
> > error bits for some commands. This is a problem for some commands, like
> > RPMB for example. Typically, we may detect that the busy completion
> > successfully has ended, while in fact the card did not complete the
> > requested operation.
> >
> > To fix the problem, let's always poll with CDM13 for these commands and
> > during the polling aggregate the R1 response bits. Before completing the
> > ioctl request, let's propagate the R1 response bits too.
> >
> > Cc: Avri Altman <avri.altman@xxxxxxx>
> > Co-developed-by: Christian Loehle <CLoehle@xxxxxxxxxxxxxx>
> > Signed-off-by: Christian Loehle <CLoehle@xxxxxxxxxxxxxx>
> > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Reviewed-by: Avri Altman <avri.altman@xxxxxxx>

Thanks!

>
> See nit below.
> Thanks,
> Avri
>
> > ---
> >
> > Christian, I took the liberty of re-working your previous patch [1]. But rather
> > than keeping your authorship I added you as a co-developer. Please tell me
> > if you prefer differently.
> >
> > Kind regards
> > Uffe
> >
> > [1]
> > https://lore.kernel.org/all/26d178dcfc2f4b7d9010145d0c051394@hypersto
> > ne.com/
> >
> > ---
> >  drivers/mmc/core/block.c | 31 ++++++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> > b6f4be25b31b..62a8aacc996c 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -179,6 +179,7 @@ static void mmc_blk_rw_rq_prep(struct
> > mmc_queue_req *mqrq,
> >                                struct mmc_queue *mq);  static void
> > mmc_blk_hsq_req_done(struct mmc_request *mrq);  static int
> > mmc_spi_err_check(struct mmc_card *card);
> > +static int mmc_blk_busy_cb(void *cb_data, bool *busy);
> >
> >  static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)  { @@ -
> > 470,7 +471,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card,
> > struct mmc_blk_data *md,
> >         struct mmc_data data = {};
> >         struct mmc_request mrq = {};
> >         struct scatterlist sg;
> > -       bool r1b_resp, use_r1b_resp = false;
> > +       bool r1b_resp;
> >         unsigned int busy_timeout_ms;
> >         int err;
> >         unsigned int target_part;
> > @@ -551,8 +552,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
> > *card, struct mmc_blk_data *md,
> >         busy_timeout_ms = idata->ic.cmd_timeout_ms ? :
> > MMC_BLK_TIMEOUT_MS;
> >         r1b_resp = (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B;
> >         if (r1b_resp)
> > -               use_r1b_resp = mmc_prepare_busy_cmd(card->host, &cmd,
> > -                                                   busy_timeout_ms);
> > +               mmc_prepare_busy_cmd(card->host, &cmd, busy_timeout_ms);
> >
> >         mmc_wait_for_req(card->host, &mrq);
> >         memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); @@ -605,19
> > +605,28 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct
> > mmc_blk_data *md,
> >         if (idata->ic.postsleep_min_us)
> >                 usleep_range(idata->ic.postsleep_min_us, idata-
> > >ic.postsleep_max_us);
> >
> > -       /* No need to poll when using HW busy detection. */
> > -       if ((card->host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
> > use_r1b_resp)
> > -               return 0;
> > -
> >         if (mmc_host_is_spi(card->host)) {
> >                 if (idata->ic.write_flag || r1b_resp || cmd.flags &
> > MMC_RSP_SPI_BUSY)
> >                         return mmc_spi_err_check(card);
> >                 return err;
> >         }
> > -       /* Ensure RPMB/R1B command has completed by polling with CMD13.
> > */
> > -       if (idata->rpmb || r1b_resp)
> > -               err = mmc_poll_for_busy(card, busy_timeout_ms, false,
> > -                                       MMC_BUSY_IO);
> > +
> > +       /*
> > +        * Ensure RPMB, writes and R1B responses are completed by polling
> > with
> > +        * CMD13. Note that, usually we don't need to poll when using HW busy
> > +        * detection, but here it's needed since some commands may indicate
> > the
> > +        * error through the R1 status bits.
> > +        */
> > +       if (idata->rpmb || idata->ic.write_flag || r1b_resp) {
> > +               struct mmc_blk_busy_data cb_data;
> > +
> > +               cb_data.card = card;
> > +               cb_data.status = 0;
> > +               err = __mmc_poll_for_busy(card->host, 0, busy_timeout_ms,
> > +                                         &mmc_blk_busy_cb, &cb_data);
> Maybe we can pack those 3 lines in an inline handler - they seems to appear a couple of times more.

You have a point, but I suggest we consider that as a potential
clean-up on top. However, I should probably update the declaration of
the struct to:

struct mmc_blk_busy_data cb_data = {
     .card = card,
     .status = 0,
};

Maybe this is a sufficient improvement?

Kind regards
Uffe