Re: [PATCH] mmc: core: Use mrq.sbc in close-ended ffu

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

On 30/10/23 11:09, 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. So for the close-ended case, it
> will be: cmd6->cmd23-cmd25-cmd6.
> 
> Some host controllers however, get confused when multi-block rw is sent
> without sbc, and may generate auto-cmd12 which breaks the 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, or hooking the request function of the msm ops,
> it would be better to fix the ioctl handling and make it use mrq.sbc
> instead of issuing SET_BLOCK_COUNT separately.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/mmc/core/block.c | 41 ++++++++++++++++++++++++++++++++++++++--
>  include/linux/mmc/mmc.h  |  1 +
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 3a8f27c3e310..6d94617ef206 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data {
>  	struct mmc_ioc_cmd ic;
>  	unsigned char *buf;
>  	u64 buf_bytes;
> +	unsigned int flags;
> +#define MMC_BLK_IOC_DROP	BIT(0)	/* drop this mrq */
> +#define MMC_BLK_IOC_SBC	BIT(1)	/* use mrq.sbc */
> +
>  	struct mmc_rpmb_data *rpmb;
>  };
>  
> @@ -479,6 +483,9 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>  	if (!card || !md || !idata)
>  		return -EINVAL;
>  
> +	if (idata->flags & MMC_BLK_IOC_DROP)
> +		return 0;
> +
>  	/*
>  	 * The RPMB accesses comes in from the character device, so we
>  	 * need to target these explicitly. Else we just target the
> @@ -532,14 +539,19 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>  			return err;
>  	}
>  
> -	if (idata->rpmb) {
> +	if (idata->rpmb || (idata->flags & MMC_BLK_IOC_SBC)) {
> +		u32 sbc_arg = data.blocks;
> +
>  		sbc.opcode = MMC_SET_BLOCK_COUNT;
>  		/*
>  		 * We don't do any blockcount validation because the max size
>  		 * may be increased by a future standard. We just copy the
>  		 * 'Reliable Write' bit here.
>  		 */
> -		sbc.arg = data.blocks | (idata->ic.write_flag & BIT(31));
> +		if (idata->rpmb)
> +			sbc_arg |= (idata->ic.write_flag & BIT(31));

What about supporting more generic use-cases with other information
in the arg of CMD23 idata, such as reliable write etc?  Perhaps
link the other idata and populate sbc opcode and arg from that.

> +
> +		sbc.arg = sbc_arg;
>  		sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
>  		mrq.sbc = &sbc;

Also could copy the sbc response back to the "CMD23" idata
after mmc_wait_for_req().

>  	}
> @@ -1032,6 +1044,28 @@ static inline void mmc_blk_reset_success(struct mmc_blk_data *md, int type)
>  	md->reset_done &= ~type;
>  }
>  
> +/* close-ended ffu */
> +static void mmc_blk_check_ce_ffu(struct mmc_queue_req *mq_rq)
> +{
> +	struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data;
> +
> +	if (mq_rq->ioc_count != 4)
> +		return;
> +
> +	if (idata[0]->ic.opcode != MMC_SWITCH)
> +		return;
> +
> +	if (MMC_EXTRACT_INDEX_FROM_ARG(idata[0]->ic.arg) !=
> +			EXT_CSD_MODE_CONFIG)
> +		return;
> +
> +	if (idata[1]->ic.opcode == MMC_SET_BLOCK_COUNT &&
> +	    idata[2]->ic.opcode == MMC_WRITE_MULTIPLE_BLOCK) {
> +		idata[1]->flags |= MMC_BLK_IOC_DROP;
> +		idata[2]->flags |= MMC_BLK_IOC_SBC;
> +	}

Could this be more generic e.g. simply

	for (i = 1; i < mq_rq->ioc_count; i++)
		if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT &&
		    mmc_op_multi(idata[i + 1]->ic.opcode)) {
			idata[i - 1]->flags |= MMC_BLK_IOC_DROP;
			idata[i]->flags |= MMC_BLK_IOC_SBC;
		}

with no need to check for 4 cmds, MMC_SWITCH or EXT_CSD_MODE_CONFIG

> +}
> +
>  /*
>   * The non-block commands come back from the block layer after it queued it and
>   * processed it with all other requests and then they get issued in this
> @@ -1059,6 +1093,9 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
>  			if (ret)
>  				break;
>  		}
> +
> +		mmc_blk_check_ce_ffu(mq_rq);
> +
>  		fallthrough;
>  	case MMC_DRV_OP_IOCTL_RPMB:
>  		idata = mq_rq->drv_op_data;
> 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 */