| Message ID | 20231025113035.1881418-1-avri.altman@wdc.com |
|---|---|
| Headers | show |
| Series | mmc: host: Disable auto-cmd12 during ffu | expand |
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(+) >
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
> 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(+) > >
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
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