mbox series

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

Message ID 20231025113035.1881418-1-avri.altman@wdc.com
Headers show
Series mmc: host: Disable auto-cmd12 during ffu | expand

Message

Avri Altman Oct. 25, 2023, 11:30 a.m. UTC
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.

v2 -> v3:
 - fix an issue Reported-by: kernel test robot <lkp@intel.com>

v1->v2:
 - Attend Adrian's suggestions

Avri Altman (2):
  mmc: core: Mark close-ended ffu in progress
  mmc: host: msm: Disable auto-cmd12 during ffu

 drivers/mmc/core/block.c     | 34 ++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci-msm.c | 24 ++++++++++++++++++++++++
 include/linux/mmc/host.h     |  1 +
 include/linux/mmc/mmc.h      |  1 +
 4 files changed, 60 insertions(+)

Comments

Adrian Hunter Oct. 26, 2023, 8:38 a.m. UTC | #1
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.

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.

> 
> v2 -> v3:
>  - fix an issue Reported-by: kernel test robot <lkp@intel.com>
> 
> v1->v2:
>  - Attend Adrian's suggestions
> 
> Avri Altman (2):
>   mmc: core: Mark close-ended ffu in progress
>   mmc: host: msm: Disable auto-cmd12 during ffu
> 
>  drivers/mmc/core/block.c     | 34 ++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci-msm.c | 24 ++++++++++++++++++++++++
>  include/linux/mmc/host.h     |  1 +
>  include/linux/mmc/mmc.h      |  1 +
>  4 files changed, 60 insertions(+)
>
Ulf Hansson Oct. 26, 2023, 10:03 a.m. UTC | #2
On Thu, 26 Oct 2023 at 10:38, Adrian Hunter <adrian.hunter@intel.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.
>
> 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.

Agree!

[...]

Kind regards
Uffe
Avri Altman Oct. 26, 2023, 10:06 a.m. UTC | #3
> 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.

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.

Please let me know if this is acceptable.

Thanks,
Avri

> 
> >
> > v2 -> v3:
> >  - fix an issue Reported-by: kernel test robot <lkp@intel.com>
> >
> > v1->v2:
> >  - Attend Adrian's suggestions
> >
> > Avri Altman (2):
> >   mmc: core: Mark close-ended ffu in progress
> >   mmc: host: msm: Disable auto-cmd12 during ffu
> >
> >  drivers/mmc/core/block.c     | 34
> ++++++++++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci-msm.c | 24 ++++++++++++++++++++++++
> >  include/linux/mmc/host.h     |  1 +
> >  include/linux/mmc/mmc.h      |  1 +
> >  4 files changed, 60 insertions(+)
> >
Ulf Hansson Oct. 27, 2023, 9:41 a.m. UTC | #4
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.

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
Adrian Hunter Oct. 27, 2023, 10:54 a.m. UTC | #5
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