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

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

[Avri Altman]

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

-- 
2.42.0

[PATCH v3 1/2] mmc: core: Mark close-ended ffu in progress

[Avri Altman]

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. So for the close-ended case, it
will be: cmd6->cmd23-cmd25-cmd6.

Capture this sequence and make it available for the host modules, should
it is needed.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/mmc/core/block.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |  1 +
 include/linux/mmc/mmc.h  |  1 +
 3 files changed, 36 insertions(+)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 3a8f27c3e310..ee5ec4686582 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -96,6 +96,10 @@ static int max_devices;
 static DEFINE_IDA(mmc_blk_ida);
 static DEFINE_IDA(mmc_rpmb_ida);
 
+static unsigned int ffu_progress;
+#define MMC_FFU_START	BIT(0)	/* FFU in progress */
+#define MMC_FFU_SBC	BIT(1)	/* close-ended FFU in progress */
+
 struct mmc_blk_busy_data {
 	struct mmc_card *card;
 	u32 status;
@@ -464,6 +468,34 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
 	return 0;
 }
 
+bool mmc_is_ffu_cmd(struct mmc_command *cmd)
+{
+	return ((ffu_progress & MMC_FFU_SBC) &&
+		cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK);
+}
+
+static void mmc_blk_ffu_progress(struct mmc_command *cmd)
+{
+	if (MMC_EXTRACT_INDEX_FROM_ARG(cmd->arg) != EXT_CSD_MODE_CONFIG &&
+	    ffu_progress == 0)
+		return;
+
+	if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd->arg) == EXT_CSD_MODE_CONFIG) &&
+	    (cmd->opcode == MMC_SWITCH)) {
+		u8 value = MMC_EXTRACT_VALUE_FROM_ARG(cmd->arg);
+
+		if (value == 1)
+			ffu_progress = MMC_FFU_START;
+		else
+			ffu_progress = 0;
+
+		return;
+	} else if (ffu_progress == MMC_FFU_START &&
+		   cmd->opcode == MMC_SET_BLOCK_COUNT) {
+		ffu_progress |= MMC_FFU_SBC;
+	}
+}
+
 static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 			       struct mmc_blk_ioc_data *idata)
 {
@@ -548,6 +580,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	    (cmd.opcode == MMC_SWITCH))
 		return mmc_sanitize(card, idata->ic.cmd_timeout_ms);
 
+	mmc_blk_ffu_progress(&cmd);
+
 	/* If it's an R1B response we need some more preparations. */
 	busy_timeout_ms = idata->ic.cmd_timeout_ms ? : MMC_BLK_TIMEOUT_MS;
 	r1b_resp = (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 2f445c651742..e2cb20f7c50c 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -677,5 +677,6 @@ int mmc_send_status(struct mmc_card *card, u32 *status);
 int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
 int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
+bool mmc_is_ffu_cmd(struct mmc_command *cmd);
 
 #endif /* LINUX_MMC_HOST_H */
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 6f7993803ee7..d4d10cabaa57 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -254,6 +254,7 @@ static inline bool mmc_ready_for_data(u32 status)
  */
 
 #define EXT_CSD_CMDQ_MODE_EN		15	/* R/W */
+#define EXT_CSD_MODE_CONFIG		30	/* R/W */
 #define EXT_CSD_FLUSH_CACHE		32      /* W */
 #define EXT_CSD_CACHE_CTRL		33      /* R/W */
 #define EXT_CSD_POWER_OFF_NOTIFICATION	34	/* R/W */
-- 
2.42.0

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

[Avri Altman]

Some platforms generate auto command error interrupt when it shouldn't,
e.g. auto-cmd12 while in close-ended ffu sequence.

I encounterd this issue while testing fwup on HP Chromebook x2, qualcomm
based QC-7c, code name for board - strongbad.

Instead of a quirk, hook the request function of the msm ops, so it'll
disable auto-cmd12 while close-ended ffu is in progress.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/mmc/host/sdhci-msm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 668e0aceeeba..99d6cac335a4 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/mmc/mmc.h>
+#include <linux/mmc/host.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_opp.h>
 #include <linux/slab.h>
@@ -2245,6 +2246,27 @@ static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
 	return -EAGAIN;
 }
 
+static void sdhci_msm_check_auto_cmd12(struct mmc_host *mmc,
+				       struct mmc_request *mrq)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	bool auto_cmd12 = (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12);
+
+	if (mrq->data && auto_cmd12) {
+		if (mmc_is_ffu_cmd(mrq->cmd))
+			host->flags &= ~SDHCI_AUTO_CMD12;
+		else
+			host->flags |= SDHCI_AUTO_CMD12;
+	}
+}
+
+static void sdhci_msm_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	sdhci_msm_check_auto_cmd12(mmc, mrq);
+
+	sdhci_request(mmc, mrq);
+}
+
 #define DRIVER_NAME "sdhci_msm"
 #define SDHCI_MSM_DUMP(f, x...) \
 	pr_err("%s: " DRIVER_NAME ": " f, mmc_hostname(host->mmc), ## x)
@@ -2638,6 +2660,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 					 MSM_MMC_AUTOSUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(&pdev->dev);
 
+	host->mmc_host_ops.request = sdhci_msm_request;
+
 	host->mmc_host_ops.start_signal_voltage_switch =
 		sdhci_msm_start_signal_voltage_switch;
 	host->mmc_host_ops.execute_tuning = sdhci_msm_execute_tuning;
-- 
2.42.0

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

[Adrian Hunter]

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

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

[Ulf Hansson]

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

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

[Avri Altman]

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

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

[Ulf Hansson]

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

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

[Adrian Hunter]

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