| Message ID | 20231030090906.1897035-1-avri.altman@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mmc: core: Use mrq.sbc in close-ended ffu | expand |
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 */
> 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. 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 */
> > > } > > > @@ -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 Or, I can follow your proposal. Will send a v3. Thanks, Avri > > 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 */
On Tue, 14 Nov 2023 at 12:21, Avri Altman <Avri.Altman@wdc.com> wrote: > > > > > } > > > > @@ -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 > Or, I can follow your proposal. > Will send a v3. Please do! I think it's better to solve this in a generic way, rather than making it specific for FFU. Kind regards Uffe
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)); + + sbc.arg = sbc_arg; sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; mrq.sbc = &sbc; } @@ -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; + } +} + /* * 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 */
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(-)