Re: [PATCH v3 0/2] mmc: host: Disable auto-cmd12 during ffu

[Adrian Hunter <adrian.hunter@intel.com>]

On 27/10/23 12:41, Ulf Hansson wrote:
> On Thu, 26 Oct 2023 at 12:07, Avri Altman <Avri.Altman@wdc.com> wrote:
>>
>>> On 25/10/23 14:30, Avri Altman wrote:
>>>> Field Firmware Update (ffu) may use close-ended or open ended sequence.
>>>> Each such sequence is comprised of a write commands enclosed between 2
>>>> switch commands - to and from ffu mode.
>>>>
>>>> Some platforms generate auto command error interrupt when it shouldn't,
>>>> e.g. auto-cmd12 while in close-ended ffu sequence.  I encountered  this
>>>> issue while testing fwupd (github.com/fwupd/fwupd) on HP Chromebook
>>> x2,
>>>> a qualcomm based QC-7c, code name - strongbad. Instead of a quirk, make
>>>> sure it disable auto-cmd12 while close-ended ffu is in progress.
>>>
>>> I think I misunderstood this because I was thinking that auto-cmd12
>>> was being used with an open-ended sequence, and that it wasn't
>>> working with FFU.  However it seems mmc-utils is using a closed-ended
>>> sequence.
>> Yes, mmc-utils, fwupd, as well as others - uses close-ended,
>> And unlike rpmb - it sends cmd23 as part of the ffu sequence.
>>
>>>
>>> It looks like the the host controller driver doesn't know that,
>>> because the ioctl interface does not use mrq.sbc and the
>>> SET_BLOCK_COUNT command is sent separately.  Then when the
>>> MULTI_WRITE
>>> command is issued, the host controller driver treats it as open-ended
>>> and will enable auto-cmd12 if the controller supports it.
>>>
>>> If that is the case, it would be better to fix the ioctl handling
>>> and make it use mrq.sbc instead of issuing SET_BLOCK_COUNT separately.
>> We can do that.
>> On the other hand, this doesn't happen on other platforms.
>> Fwupd has just recently switched to close-ended, but mmc-utils is using close-ended mode for many years,
>> Performing ffu successfully on many different platforms.
>> My understanding is, that the hw should realize that cmd23 has just sent prior to cmd25 and avoid this auto-cmd12.
> 
> Yes, in principle that's correct.
> 
> In fact, I think that most host drivers should already support this
> behavior, although it relies on the CMD23 to be incorporated within
> the same mmc request (mrq) as the CMD25. We use "mrq.sbc" for this and
> the host driver uses MMC_CAP_CMD23 to inform the MMC core whether it
> supports this or not.
> 
>>
>> Going back to your proposal, we can ignore cmd23 in close-ended ffu, but eventually,
>> we will need to change mmc-utils and fwupd to stop send cmd23.
> 
> This is not what we proposed, at least if I understood Adrian correctly.
> 
> Instead, the idea that could make better sense, is to fix the mmc
> ioctl handling in the mmc core, so that it can discover that a CMD23
> command is followed by another CMD18/25 (multiple read/write). And in
> this case, it should boundle the commands together, using mrq.sbc so
> that one request gets sent to the mmc host driver instead of two.

Yes that is what I was thinking.  Perhaps look at
__mmc_blk_ioctl_cmd() first.  It doesn't have enough information
to decide what to do, so either something needs to be added to
struct mmc_blk_ioc_data and set up before hand, or it needs to
be passed struct mmc_queue_req *mq_rq.

> 
> In this way, there should be no need for any specific changes to any
> of the host drivers (assuming they have the CMD23 support implemented
> correctly), they should just work.
> 
> [...]
> 
> Kind regards
> Uffe