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@xxxxxxx>
> ---
> 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 */
