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

[PATCH 0/3] 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.  Therefore, add a
quirk that disable auto-cmd12 while close-ended ffu is in progress.

I encounterd this issue while testing fwupd (github.com/fwupd/fwupd) on
HP Chromebook x2, a qualcomm based QC-7c, code name - strongbad.

Avri Altman (3):
  mmc: host: Quirk - Disable auto-cmd12 during ffu
  mmc: core: Mark close-ended ffu in progress
  mmc: host: sdhci-msm: Apply SDHCI_QUIRK2_FFU_ACMD12

 drivers/mmc/core/block.c     | 25 +++++++++++++++++++++++++
 drivers/mmc/host/sdhci-msm.c |  3 ++-
 drivers/mmc/host/sdhci.c     |  8 +++++++-
 drivers/mmc/host/sdhci.h     |  2 ++
 include/linux/mmc/core.h     |  3 +++
 include/linux/mmc/mmc.h      |  1 +
 6 files changed, 40 insertions(+), 2 deletions(-)

-- 
2.42.0

[PATCH 1/3] mmc: host: Quirk - 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.

Therefore, add a quirk that disable auto-cmd12 while close-ended ffu is
in progress.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ff41aa56564e..6d58f71f926e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1389,11 +1389,17 @@ void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
 }
 EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
 
+static inline bool sdhci_dont_auto_cmd12_ffu(struct sdhci_host *host,
+					     struct mmc_request *mrq) {
+	return (host->quirks2 & SDHCI_QUIRK2_FFU_ACMD12) && mrq->ffu;
+}
+
 static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
 				    struct mmc_request *mrq)
 {
 	return !mrq->sbc && (host->flags & SDHCI_AUTO_CMD12) &&
-	       !mrq->cap_cmd_during_tfr;
+	       !mrq->cap_cmd_during_tfr &&
+	       !sdhci_dont_auto_cmd12_ffu(host, mrq);
 }
 
 static inline bool sdhci_auto_cmd23(struct sdhci_host *host,
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index f219bdea8f28..ccab3a76883c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -485,6 +485,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
 /* Issue CMD and DATA reset together */
 #define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER	(1<<19)
+/* Controller generates auto-cmd error interrupt during close-ended ffu */
+#define SDHCI_QUIRK2_FFU_ACMD12				(1<<20)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 6efec0b9820c..aca4e84f648c 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -161,6 +161,9 @@ struct mmc_request {
 	/* Allow other commands during this ongoing data transfer or busy wait */
 	bool			cap_cmd_during_tfr;
 
+	/* Be aware that ffu is in progress and avoid auto-cmd12 */
+	bool			ffu;
+
 	int			tag;
 
 #ifdef CONFIG_MMC_CRYPTO
-- 
2.42.0

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

[Avri Altman]

The SDHCI_QUIRK2_FFU_ACMD12 quirk was invented to prevent from those
bogus sdhci to use auto-cmd12 after cmd25 in a close-ended ffu process.
Capture the applicable mrq and mark it so it won't use auto-cmd12 post
that cmd25.

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 3a8f27c3e310..d92e7322c6da 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -114,6 +114,8 @@ struct mmc_blk_data {
 	unsigned int	flags;
 #define MMC_BLK_CMD23	(1 << 0)	/* Can do SET_BLOCK_COUNT for multiblock */
 #define MMC_BLK_REL_WR	(1 << 1)	/* MMC Reliable write support */
+#define MMC_BLK_FFU	(1 << 2)	/* FFU in progress */
+#define MMC_BLK_CE	(1 << 3)	/* close-ended FFU in progress */
 
 	struct kref	kref;
 	unsigned int	read_only;
@@ -548,6 +550,29 @@ 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);
 
+	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) {
+			md->flags |= MMC_BLK_FFU;
+		} else if (value == 0) {
+			/* switch back to normal mode is always happening */
+			md->flags &= ~MMC_BLK_FFU;
+			md->flags &= ~MMC_BLK_CE;
+		}
+	}
+
+	if ((md->flags & MMC_BLK_FFU) && cmd.opcode == MMC_SET_BLOCK_COUNT) {
+		md->flags &= ~MMC_BLK_FFU;
+		md->flags |= MMC_BLK_CE;
+	}
+
+	if ((md->flags & MMC_BLK_CE) && mmc_op_multi(cmd.opcode)) {
+		mrq.ffu = true;
+		md->flags &= ~MMC_BLK_CE;
+	}
+
 	/* 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/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 3/3] mmc: host: sdhci-msm: Apply SDHCI_QUIRK2_FFU_ACMD12

[Avri Altman]

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

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

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 668e0aceeeba..78a658c22d03 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2339,7 +2339,8 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
 		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
 		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
 
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+		   SDHCI_QUIRK2_FFU_ACMD12,
 	.ops = &sdhci_msm_ops,
 };
 
-- 
2.42.0

Re: [PATCH 1/3] mmc: host: Quirk - Disable auto-cmd12 during ffu

[Adrian Hunter]

On 12/10/23 21:40, 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.
> 
> Therefore, add a quirk that disable auto-cmd12 while close-ended ffu is
> in progress.

If you hook the request function, maybe the existing SDHCI_AUTO_CMD12
flag could be used instead of introducing a new quirk:

void sdhci_msm_request(etc)
{
	if data-mrq and using-auto-cmd-12
		if ffu
			host->flags &= ~SDHCI_AUTO_CMD12;
		else
			host->flags |= SDHCI_AUTO_CMD12;
	sdhci_request(etc)
}

> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/mmc/host/sdhci.c | 8 +++++++-
>  drivers/mmc/host/sdhci.h | 2 ++
>  include/linux/mmc/core.h | 3 +++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ff41aa56564e..6d58f71f926e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1389,11 +1389,17 @@ void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
>  }
>  EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
>  
> +static inline bool sdhci_dont_auto_cmd12_ffu(struct sdhci_host *host,
> +					     struct mmc_request *mrq) {
> +	return (host->quirks2 & SDHCI_QUIRK2_FFU_ACMD12) && mrq->ffu;
> +}
> +
>  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
>  				    struct mmc_request *mrq)
>  {
>  	return !mrq->sbc && (host->flags & SDHCI_AUTO_CMD12) &&
> -	       !mrq->cap_cmd_during_tfr;
> +	       !mrq->cap_cmd_during_tfr &&
> +	       !sdhci_dont_auto_cmd12_ffu(host, mrq);
>  }
>  
>  static inline bool sdhci_auto_cmd23(struct sdhci_host *host,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index f219bdea8f28..ccab3a76883c 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -485,6 +485,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
>  /* Issue CMD and DATA reset together */
>  #define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER	(1<<19)
> +/* Controller generates auto-cmd error interrupt during close-ended ffu */
> +#define SDHCI_QUIRK2_FFU_ACMD12				(1<<20)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 6efec0b9820c..aca4e84f648c 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -161,6 +161,9 @@ struct mmc_request {
>  	/* Allow other commands during this ongoing data transfer or busy wait */
>  	bool			cap_cmd_during_tfr;
>  
> +	/* Be aware that ffu is in progress and avoid auto-cmd12 */
> +	bool			ffu;
> +
>  	int			tag;
>  
>  #ifdef CONFIG_MMC_CRYPTO

Re: [PATCH 2/3] mmc: core: Mark close-ended ffu in progress

[Adrian Hunter]

On 12/10/23 21:40, Avri Altman wrote:
> The SDHCI_QUIRK2_FFU_ACMD12 quirk was invented to prevent from those
> bogus sdhci to use auto-cmd12 after cmd25 in a close-ended ffu process.
> Capture the applicable mrq and mark it so it won't use auto-cmd12 post
> that cmd25.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/mmc/core/block.c | 25 +++++++++++++++++++++++++
>  include/linux/mmc/mmc.h  |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 3a8f27c3e310..d92e7322c6da 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -114,6 +114,8 @@ struct mmc_blk_data {
>  	unsigned int	flags;
>  #define MMC_BLK_CMD23	(1 << 0)	/* Can do SET_BLOCK_COUNT for multiblock */
>  #define MMC_BLK_REL_WR	(1 << 1)	/* MMC Reliable write support */
> +#define MMC_BLK_FFU	(1 << 2)	/* FFU in progress */
> +#define MMC_BLK_CE	(1 << 3)	/* close-ended FFU in progress */
>  
>  	struct kref	kref;
>  	unsigned int	read_only;
> @@ -548,6 +550,29 @@ 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);
>  
> +	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) {
> +			md->flags |= MMC_BLK_FFU;
> +		} else if (value == 0) {
> +			/* switch back to normal mode is always happening */
> +			md->flags &= ~MMC_BLK_FFU;
> +			md->flags &= ~MMC_BLK_CE;
> +		}
> +	}
> +
> +	if ((md->flags & MMC_BLK_FFU) && cmd.opcode == MMC_SET_BLOCK_COUNT) {
> +		md->flags &= ~MMC_BLK_FFU;
> +		md->flags |= MMC_BLK_CE;
> +	}
> +
> +	if ((md->flags & MMC_BLK_CE) && mmc_op_multi(cmd.opcode)) {
> +		mrq.ffu = true;
> +		md->flags &= ~MMC_BLK_CE;
> +	}

Could it be a separate helper function so it ends up like

	mrq.ffu = mmc_is_ffu_cmd(&cmd)

But perhaps then mrq.ffu is not needed and sdhci-msm.c
could call mmc_is_ffu_cmd() directly?

> +
>  	/* 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/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 */

RE: [PATCH 1/3] mmc: host: Quirk - Disable auto-cmd12 during ffu

[Avri Altman]

> 
> On 12/10/23 21:40, 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.
> >
> > Therefore, add a quirk that disable auto-cmd12 while close-ended ffu
> > is in progress.
> 
> If you hook the request function, maybe the existing SDHCI_AUTO_CMD12
> flag could be used instead of introducing a new quirk:
> 
> void sdhci_msm_request(etc)
> {
>         if data-mrq and using-auto-cmd-12
>                 if ffu
>                         host->flags &= ~SDHCI_AUTO_CMD12;
>                 else
>                         host->flags |= SDHCI_AUTO_CMD12;
>         sdhci_request(etc)
> }
Thanks - will do.

Thanks,
Avri

RE: [PATCH 2/3] mmc: core: Mark close-ended ffu in progress

[Avri Altman]

> >
> > +     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) {
> > +                     md->flags |= MMC_BLK_FFU;
> > +             } else if (value == 0) {
> > +                     /* switch back to normal mode is always happening */
> > +                     md->flags &= ~MMC_BLK_FFU;
> > +                     md->flags &= ~MMC_BLK_CE;
> > +             }
> > +     }
> > +
> > +     if ((md->flags & MMC_BLK_FFU) && cmd.opcode ==
> MMC_SET_BLOCK_COUNT) {
> > +             md->flags &= ~MMC_BLK_FFU;
> > +             md->flags |= MMC_BLK_CE;
> > +     }
> > +
> > +     if ((md->flags & MMC_BLK_CE) && mmc_op_multi(cmd.opcode)) {
> > +             mrq.ffu = true;
> > +             md->flags &= ~MMC_BLK_CE;
> > +     }
> 
> Could it be a separate helper function so it ends up like
> 
>         mrq.ffu = mmc_is_ffu_cmd(&cmd)
> 
> But perhaps then mrq.ffu is not needed and sdhci-msm.c could call
> mmc_is_ffu_cmd() directly?
Yes.  Will do.

Thanks,
Avri