Re: [PATCHv2] mmc: block: Differentiate busy and PROG state

["Christian Löhle" <CLoehle@hyperstone.com>]

>>
>> Prevent race condition with ioctl commands
>>
>> To fully prevent a race condition where the next
>> issued command will be rejected as the card is no
>> longer signalling busy but not yet back in TRAN state.
>> The card may be in PROG state without signalling busy,
>> for some of the commands that are only R1, but also
>> for R1b commands, the card will signal non-busy as soon
>> as receive buffers are free again, but the card has
>> not finished handling the command and may therefore be
>> in PROG.
>
>Can you please point me to the corresponding information in the spec
>that states that the above behavior is correct?

Sure, unfortunately it is blanked in the simplified spec so I (think I) cannot simply cite it now.
If access to the full spec is a problem for many contributors of the list, I would reconsider the legal aspects.
The part I'm referring to is the last (two) sentence(s) of the section "Single Block Write" in 4.12.3 Data Write.
(Single Block Write is the correct section as all R1 (no b) commands with data behave like Single Block Write.
This info itself is scattered throughout the (simplified) spec, but for CMD42 it is:
"The card lock/unlock command has the structure and bus transaction type of a regular single block write command."
For some other commands it is in the command description column of the 4.7.4)

>
>In principle what you are saying is that busy signalling on DAT0 is
>*entirely* broken, at least for some cards and some commands.

I wouldn't say broken, it seems to do what is described.
But right now busy polling is essentially CMD13 polling, right? at least for card_busy_detect.
So it doesn't hurt polling for TRAN.

But not for all commands busy signalling <=> PROG state is true:
"The card may provide buffering for block write. This means that the next block can be sent to the card while the previous is being programmed." (4.3)
"There is no buffering option for write CSD, write protection and erase. This means that while the card is busy servicing any one of these commands, no other data transfer commands will be accepted. DAT0 line will be kept low as long as the card is busy and in the Programming State." (4.3)
Definitely leaves us with MMC_PROGRAM_CID, CMD20, CMD42, MMC_SET_TIME, MMC_GEN_CMD and both SD_WRITE_EXTR.
Furthermore I think the cleaner solution is to poll for TRAN anyway. Just in theory there does not seem to be a timing constraint for when the card starts signalling busy for R1 (non-b) commands.
Just in theory the ioctl handling could be done BEFORE the card ever moves to PROG and starts signalling busy. (Im not sure about this though)

>>
>> -static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
>> +static int is_prog_cmd(struct mmc_command *cmd)
>> +{
>> +       /*
>> +        * Cards will move to programming state (PROG) after these commands.
>> +        * So we must not consider the command as completed until the card
>> +        * has actually returned back to TRAN state.
>> +        */
>> +       switch (cmd->opcode) {
>> +       case MMC_STOP_TRANSMISSION:
>
>This has an R1B response, hence we already do the proper polling that is needed.
>
>In other words, we don't need to explicitly check for this command
>here, as we are already checking the response type (R1B) in
>__mmc_blk_ioctl_cmd().

Fair. I will happily remove any multi block write commands.
Just seemed like the safer way to do so, but I have not specifically checked if any cards violate this here.

>
>> +       case MMC_WRITE_DAT_UNTIL_STOP:
>
>What's this used for? It's obsolete, at least in the eMMC spec. Please drop it.

Okay.

>> +       case MMC_WRITE_BLOCK:
>> +       case MMC_WRITE_MULTIPLE_BLOCK:
>
>These are already supported via the generic block interface, please
>drop the checks.

Might make sense to let userspace issue read/write, too.
But I understand if this is not desired.

>> +       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.
>
>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.
>
>> +       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)

>
>> +       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.


>
>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?


Kind Regards,
Christian
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782