On 06/03/24 10:44:38, Dominique Martinet wrote:
From: Dominique Martinet dominique.martinet@atmark-techno.com
Commit e7794c14fd73 ("mmc: rpmb: fixes pause retune on all RPMB partitions.") added a mask check for 'part_type', but the mask used was wrong leading to the code intended for rpmb also being executed for GP3.
On some MMCs (but not all) this would make gp3 partition inaccessible: armadillo:~# head -c 1 < /dev/mmcblk2gp3 head: standard input: I/O error armadillo:~# dmesg -c [ 422.976583] mmc2: running CQE recovery [ 423.058182] mmc2: running CQE recovery [ 423.137607] mmc2: running CQE recovery [ 423.137802] blk_update_request: I/O error, dev mmcblk2gp3, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 4 prio class 0 [ 423.237125] mmc2: running CQE recovery [ 423.318206] mmc2: running CQE recovery [ 423.397680] mmc2: running CQE recovery [ 423.397837] blk_update_request: I/O error, dev mmcblk2gp3, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0 [ 423.408287] Buffer I/O error on dev mmcblk2gp3, logical block 0, async page read
the part_type values of interest here are defined as follow: main 0 boot0 1 boot1 2 rpmb 3 gp0 4 gp1 5 gp2 6 gp3 7
right, the patch I originally sent didn't consider anything after GP0 as per the definitions below.
#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7) #define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1) #define EXT_CSD_PART_CONFIG_ACC_RPMB (0x3) #define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4)
That looked strange as there should be support for 4 GP but this code kind of convinced me of the opposite.
if (idata->rpmb) { /* Support multiple RPMB partitions */ target_part = idata->rpmb->part_index; target_part |= EXT_CSD_PART_CONFIG_ACC_RPMB; }
So if we apply the fix that you propose, how are multiple RPMB partitions (ie, 4) going to be identified as RPMB? Unless there can't be more than 3?
But sure, your patch makes sense to me.
so mask with EXT_CSD_PART_CONFIG_ACC_MASK (7) to correctly identify rpmb
Fixes: e7794c14fd73 ("mmc: rpmb: fixes pause retune on all RPMB partitions.") Cc: stable@vger.kernel.org Cc: Jorge Ramirez-Ortiz jorge@foundries.io Signed-off-by: Dominique Martinet dominique.martinet@atmark-techno.com
A couple of notes:
- this doesn't fail on all eMMCs, I can still access gp3 on some models but it seems to fail reliably with micron's "G1M15L"
- I've encountered this on the 5.10 backport (in 5.10.208), so that'll need to be backported everywhere the fix was taken...
Thanks!
drivers/mmc/core/block.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 32d49100dff5..86efa6084696 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -874,10 +874,11 @@ 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;
- const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_MASK;
- const unsigned int rpmb = EXT_CSD_PART_CONFIG_ACC_RPMB; int ret = 0;
- if ((part_type & mask) == mask) {
- if ((part_type & mask) == rpmb) { if (card->ext_csd.cmdq_en) { ret = mmc_cmdq_disable(card); if (ret)
@@ -892,10 +893,11 @@ 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;
- const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_MASK;
- const unsigned int rpmb = EXT_CSD_PART_CONFIG_ACC_RPMB; int ret = 0;
- if ((part_type & mask) == mask) {
- if ((part_type & mask) == rpmb) { mmc_retune_unpause(card->host); if (card->reenable_cmdq && !card->ext_csd.cmdq_en) ret = mmc_cmdq_enable(card);
base-commit: 5847c9777c303a792202c609bd761dceb60f4eed change-id: 20240306-mmc-partswitch-c3a50b5084ae
Best regards,
Dominique Martinet | Asmadeus