| Message ID | 20231201091034.936441-1-jorge@foundries.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: rpmb: fixes pause retune on all RPMB partitions. | expand |
Hi Jorge, thanks for your patch! On Fri, Dec 1, 2023 at 10:10 AM Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > When RPMB was converted to a character device, it added support for > multiple RPMB partitions (Commit 97548575bef3 ("mmc: block: Convert RPMB > to a character device"). > > One of the changes in this commit was transforming the variable > target_part defined in __mmc_blk_ioctl_cmd into a bitmask. > > This inadvertedly regressed the validation check done in > mmc_blk_part_switch_pre() and mmc_blk_part_switch_post(). > > This commit fixes that regression. > > Fixes: 97548575bef3 ("mmc: block: Convert RPMB to a character device") > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> My bug :/ Shouldn't we also add Cc: stable@vger.kernel.org? > + const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB; > int ret = 0; > > - if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) { > + if (part_type & mask == mask) { That looks complex, can't we just: if (part_type & EXT_CSD_PART_CONFIG_ACC_RPMB)? > + const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB; > int ret = 0; > > - if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) { > + if (part_type & mask == mask) { Dito here. Yours, Linus Walleij
On 01/12/23 10:28:52, Linus Walleij wrote: > Hi Jorge, > > thanks for your patch! > > On Fri, Dec 1, 2023 at 10:10 AM Jorge Ramirez-Ortiz <jorge@foundries.io> wrote: > > > When RPMB was converted to a character device, it added support for > > multiple RPMB partitions (Commit 97548575bef3 ("mmc: block: Convert RPMB > > to a character device"). > > > > One of the changes in this commit was transforming the variable > > target_part defined in __mmc_blk_ioctl_cmd into a bitmask. > > > > This inadvertedly regressed the validation check done in > > mmc_blk_part_switch_pre() and mmc_blk_part_switch_post(). > > > > This commit fixes that regression. > > > > Fixes: 97548575bef3 ("mmc: block: Convert RPMB to a character device") > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > My bug :/ > Shouldn't we also add Cc: stable@vger.kernel.org? ack, will do. > > > > + const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB; > > int ret = 0; > > > > - if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) { > > + if (part_type & mask == mask) { > > That looks complex, can't we just: > > if (part_type & EXT_CSD_PART_CONFIG_ACC_RPMB)? I chose to mention the mask nature of the field for clarity - just in case - but I'd much rather do your suggestion. So will do :) > > > + const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB; > > int ret = 0; > > > > - if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) { > > + if (part_type & mask == mask) { > > Dito here. yep > > Yours, > Linus Walleij thanks !
On Fri, Dec 1, 2023 at 10:47 AM Jorge Ramirez <jorge@foundries.io> wrote: > On Fri, Dec 1, 2023 at 10:39 AM Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io> wrote: >> On 01/12/23 10:28:52, Linus Walleij wrote: >> > > + const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB; >> > > int ret = 0; >> > > >> > > - if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) { >> > > + if (part_type & mask == mask) { >> > >> > That looks complex, can't we just: >> > >> > if (part_type & EXT_CSD_PART_CONFIG_ACC_RPMB)? >> >> >> I chose to mention the mask nature of the field for clarity - just in >> case - but I'd much rather do your suggestion. So will do :) > > > sorry no, I mispoke (I like clean code so yours looked neat) > we have to compare against EXT_CSD_PART_CONFIG_ACC_RPMB > bitfield since part_type could be EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1) > in which case we have to skip it. Aha those defines are not flags but enumerators. I get it. Yours, Linus Walleij
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 152dfe593c43..8d29687635c4 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -851,9 +851,10 @@ static const struct block_device_operations mmc_bdops = { static int mmc_blk_part_switch_pre(struct mmc_card *card, unsigned int part_type) { + const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB; int ret = 0; - if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) { + if (part_type & mask == mask) { if (card->ext_csd.cmdq_en) { ret = mmc_cmdq_disable(card); if (ret) @@ -868,9 +869,10 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card, static int mmc_blk_part_switch_post(struct mmc_card *card, unsigned int part_type) { + const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB; int ret = 0; - if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) { + if (part_type & mask == mask) { mmc_retune_unpause(card->host); if (card->reenable_cmdq && !card->ext_csd.cmdq_en) ret = mmc_cmdq_enable(card);
When RPMB was converted to a character device, it added support for multiple RPMB partitions (Commit 97548575bef3 ("mmc: block: Convert RPMB to a character device"). One of the changes in this commit was transforming the variable target_part defined in __mmc_blk_ioctl_cmd into a bitmask. This inadvertedly regressed the validation check done in mmc_blk_part_switch_pre() and mmc_blk_part_switch_post(). This commit fixes that regression. Fixes: 97548575bef3 ("mmc: block: Convert RPMB to a character device") Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> --- drivers/mmc/core/block.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) -- 2.34.1