> 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.
Actually ffu sequence comes as a close, well-defined form.
Except ffu & rpmb, I am un-aware of any other use-case that apply here.
Thus, I wish to make it anything but generic - as specific as possible for close-ended ffu.
Saying that, I came to realize that ffu doesn't use reliable write (strange, isn't it?),
So sbc.arg = data.blocks | (idata->ic.write_flag & BIT(31)) can stay as it is.
Will fix this in v2.
>
> > +
> > + 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)) {
I guess you meant (idata[i]
> 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
Ditto
Thanks,
Avri
>
> > +}
> > +
> > /*
> > * 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 */
