Received-SPF: pass (google.com: domain of linux-kernel+bounces-93852-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 6 Mar 2024 20:39:29 +0900
From: Dominique Martinet <asmadeus@codewreck.org>
To: "Jorge Ramirez-Ortiz, Foundries" <jorge@foundries.io>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>, linux-mmc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Dominique Martinet <dominique.martinet@atmark-techno.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] mmc: part_switch: fixes switch on gp3 partition
Message-ID: <ZehV8RrGdM9a1hO4@codewreck.org>
References: <20240306-mmc-partswitch-v1-1-bf116985d950@codewreck.org>
 <Zegx5PCtg6hs8zyp@trax>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Zegx5PCtg6hs8zyp@trax>

Jorge Ramirez-Ortiz, Foundries wrote on Wed, Mar 06, 2024 at 10:05:40AM +0100:
> > 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)

Yes, as far as I can see these are used in drivers/mmc/core/mmc.c
for example for GP0, below snippet:
                        mmc_part_add(card, part_size << 19,
                                EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
                                "gp%d", idx, false,
                                MMC_BLK_DATA_AREA_GP);

where idx is in [0;3], so we've got 4-7 for GP partitions in the part's
part_cfg.

(similarly, boot has BOOT0 + [0-1], and RPMB has RPMB without anything
added -- so as far as this field is concerned there seems to be a single
RPMB)

> 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?

Hmm, that code is definitely odd.
Reading this I'd normally assume that idata->rpmb->part_index ought to
be in a range separate fom EXT_CSD_PART_CONFIG_ACC_MASK -- so we've got
the ACC_RPMB "flag" that identifies it as RPMB within the mask, and then
it can target a given index within that.

But as far as I'm seeing in the code, rpmb->part_index always comes from
mmc_blk_alloc_rpmb_part (set to part's part_cfg), which in turn is only
called if area_type is MMC_BLK_DATA_AREA_RPMB, which is only set for
mmc_part_add() for rpmb with ACC_RPMB as part_index.. So we've got
target_part = 3 and then target_part |= 3 which will leave the value
unchanged.

Even assuming part_index was something else than 3 (let's say 1 or 2),
we'd end up with target_part = 4 or 5 which won't match the
PART_CONFIG_ACC_MASK check (&3 != 3), so it doesn't make sense until
something is shifted somewhere outside of the mask, and I see no trace
of part_index being shifted.

So the if (idata->rpmb) itself makes sense as per the comment, but we
could just have target_part take either values here as far as I
understand.


I've never actually used the rpmb partition of my MMCs so I'm not sure
how multiple RPMB partitions is supposed to work in the first place,
sorry.
That code is authored by Linus W (in 2017), perhaps he'll remember
something?

-- 
Dominique