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
