Message-ID: <CAPDyKFoU11viht4jbK5OVfW_o2e1shB4DWLT5_Jk0gMSBXwgNg@mail.gmail.com>
Date:   Mon, 9 Aug 2021 17:16:19 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Christian Löhle <CLoehle@...erstone.com>
Cc:     Avri Altman <Avri.Altman@....com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hch@...radead.org" <hch@...radead.org>
Subject: Re: [PATCHv2] mmc: block: Differentiate busy and PROG state

[...]

> >> +       case MMC_PROGRAM_CID:
> >> +       case MMC_PROGRAM_CSD:
> >
> >Let's discuss these, since they have R1 responses.
> >
> >Although, according to the eMMC spec, the card moves to rcv state, not
> >the prg state as you refer to in the commit message. Normally, we
> >don't need to poll for busy/tran completion of these commands.
>
> Why not? Sure they move to rcv first, but if data stops they move to PROG.

Yes, I guess they could.

> >
> >Have you observed through proper tests that this is actually needed?
>
> No, seems unlikely to hit this, as PROG will likely be shorter than getting a second command through.

Right.

In any case, I wouldn't mind if we start to poll for these, it sure
sounds a bit more robust.

> >
> >> +       case MMC_SET_WRITE_PROT:
> >> +       case MMC_CLR_WRITE_PROT:
> >> +       case MMC_ERASE:
> >
> >The three above have R1B, please drop them from here as they are
> >already supported correctly.
> >
> >> +       case MMC_LOCK_UNLOCK:
> >
> >Again, this has an R1 response and the card moves to rcv state.
> >Normally we shouldn't need to poll, but I have to admit that the eMMC
> >spec isn't really clear on what will happen when using the "forced
> >erase" argument. The spec mentions a 3 minute timeout....
>
> Again I don't know why you would not need to poll.
> The force erase has a good reason to remain in PROG for long, but whatever, a card may decide to just take 5 seconds in unlock PROG. (to prevent bruteforcing passwords lets say)(Not anything I have seen or expect to see)

As I said, the spec is not really clear here. So yes, polling sounds
more safe/robust.

>
> >
> >> +       case MMC_SET_TIME: /* Also covers SD_WRITE_EXTR_SINGLE */
> >> +       case MMC_GEN_CMD:
> >> +       case SD_WRITE_EXTR_MULTI:
> >
> >Are these actually being used? If not, please drop them from being
> >supported. I don't want to encourage crazy operations being issued
> >from userspace.
>
> GEN_CMD is extremly interesting for issuing vendor commands from user-space.
> Not sure if anyone uses it (yet), but if so it's unlikely to be seen in the wild.
> SD_WRITE_EXTR_MULTI is simply too new to really say.
> MMC_SET_TIME probably not used.
>

I am worried that it looks like we encourage doing "crazy" things from
userspace, when we add explicit checks for these commands.

In any case, if you strongly think we need these, please add polling
for them (or a subset of them).

>
> >
> >Overall, it looks like we need to add a check for MMC_LOCK_UNLOCK to
> >poll for busy, but that's it, I think.
>
> See above.
>
>
> >>         } while (!mmc_ready_for_data(status));
> >
> >I don't quite understand what we accomplish with polling for TRAN
> >state in one case and in the other case, both TRAN and READY_FOR_DATA.
> >Why can't we always poll for TRAN and READY_FOR_DATA? It should work
> >for all cases, no?
> >
>
> Well in theory you're then dropping the buffered writing feature of the SD spec if waiting for TRAN, too.
> I'm fine with that, especially since it is not desired to be used through ioctl anyway?

Well, I think you are misinterpreting how the buffered writing feature
is working in practice.

The mmc core doesn't write one block at the time and polls for
READY_FOR_DATA/TRAN in between each written block. Instead, it's the
responsibility of the mmc controller HW, to monitor DAT0 during a data
transmission, to understand whether it can continue to fill and write
more buffers.

Kind regards
Uffe

Powered by blists - more mailing lists