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

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>
---

Changelog:
v2--v3:
	Adopt Adrian's proposal
v1--v2:
	remove redundant reference of reliable write
---
 drivers/mmc/core/block.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

On 28/11/23 16:54, 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>
> ---
> 
> Changelog:
> v2--v3:
> 	Adopt Adrian's proposal
> v1--v2:
> 	remove redundant reference of reliable write
> ---
>  drivers/mmc/core/block.c | 40 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index f9a5cffa64b1..927257a5e8c2 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;
>  };
>  
> @@ -465,7 +469,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
>  }
>  
>  static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
> -			       struct mmc_blk_ioc_data *idata)
> +			       struct mmc_blk_ioc_data **idatas, int i)
>  {
>  	struct mmc_command cmd = {}, sbc = {};
>  	struct mmc_data data = {};
> @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>  	unsigned int busy_timeout_ms;
>  	int err;
>  	unsigned int target_part;
> +	struct mmc_blk_ioc_data *idata = idatas[i];
> +	struct mmc_blk_ioc_data *prev_idata = NULL;
>  
>  	if (!card || !md || !idata)
>  		return -EINVAL;
>  
> +	if (idata->flags & MMC_BLK_IOC_DROP)
> +		return 0;
> +
> +	if (idata->flags & MMC_BLK_IOC_SBC)
> +		prev_idata = idatas[i - 1];
> +
>  	/*
>  	 * The RPMB accesses comes in from the character device, so we
>  	 * need to target these explicitly. Else we just target the
> @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>  			return err;
>  	}
>  
> -	if (idata->rpmb) {
> +	if (idata->rpmb || prev_idata) {
>  		sbc.opcode = MMC_SET_BLOCK_COUNT;
>  		/*
>  		 * We don't do any blockcount validation because the max size
> @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>  		 * 'Reliable Write' bit here.
>  		 */
>  		sbc.arg = data.blocks | (idata->ic.write_flag & BIT(31));
> +		if (prev_idata)
> +			sbc.arg = prev_idata->ic.arg;
>  		sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
>  		mrq.sbc = &sbc;
>  	}
> @@ -557,6 +571,9 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>  	mmc_wait_for_req(card->host, &mrq);
>  	memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp));
>  
> +	if (prev_idata)
> +		memcpy(&prev_idata->ic.response, sbc.resp, sizeof(sbc.resp));

Need to check sbc.error also in this case, otherwise
looks good to me.

> +
>  	if (cmd.error) {
>  		dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
>  						__func__, cmd.error);
> @@ -1032,6 +1049,20 @@ static inline void mmc_blk_reset_success(struct mmc_blk_data *md, int type)
>  	md->reset_done &= ~type;
>  }
>  
> +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq)
> +{
> +	struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data;
> +	int i;
> +
> +	for (i = 1; i < mq_rq->ioc_count; i++) {
> +		if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT &&
> +		    mmc_op_multi(idata[i]->ic.opcode)) {
> +			idata[i - 1]->flags |= MMC_BLK_IOC_DROP;
> +			idata[i]->flags |= MMC_BLK_IOC_SBC;
> +		}
> +	}
> +}
> +
>  /*
>   * 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,11 +1090,14 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
>  			if (ret)
>  				break;
>  		}
> +
> +		mmc_blk_check_sbc(mq_rq);
> +
>  		fallthrough;
>  	case MMC_DRV_OP_IOCTL_RPMB:
>  		idata = mq_rq->drv_op_data;
>  		for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) {
> -			ret = __mmc_blk_ioctl_cmd(card, md, idata[i]);
> +			ret = __mmc_blk_ioctl_cmd(card, md, idata, i);
>  			if (ret)
>  				break;
>  		}