Thanks for the review!

Linus Walleij wrote on Wed, Mar 06, 2024 at 09:03:45AM +0100:

...

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

Which device is this?

Sorry I gave the 'name' as seen in linux, the actual reference is MTFC32GAZAQHD-IT (but we're creating the GP manually)

I have never seen an eMMC using the GP:s in my life.

Or did you create the GP manually?

Yes, we're creating them on all our devices: (japanese only, but you might get something out of google translate or similar if interested: https://armadillo.atmark-techno.com/about )

I'd be hard pressed to explain why other than "it's always been done" and I've been replicating that pattern, but as far as I understand the gp partitions are always created in "write reliable" mode so it might help a bit with lifetime if the main area is left as default.

We're using them for rare, important logs and things like that (minimal trace of updates, hard reset etc for diagnostic)

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?

On Wed, Mar 6, 2024 at 10:05 AM Jorge Ramirez-Ortiz, Foundries jorge@foundries.io wrote:

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?

As far as I can tell there can only be one RPMB partition per device.

The v5.1A spec says (section 6.2.1):

"Two Boot Area Partitions, (...)" "One RPMB Partition accessed through a trusted mechanism, (...)" "Four General Purpose Area Partitions (...)"

implying there can be only one RPMB.

Also I have never seen more than one in practice.

I paged in Jens Wiklander and Tomas Winkler who used it much more than I have, to confirm.

I think my linked list of RPMB partitions is a case of overdesign and could be simplified. I blame the fact that I didn't have the (non-public) spec at the time.

Yours, Linus Walleij

On Wed, Mar 6, 2024 at 3:22 PM Jorge Ramirez-Ortiz, Foundries jorge@foundries.io wrote:

I still cant grasp how "target_part = idata->rpmb->part_index" is helping in the design.

What happens when:

1. EXT_CSD_PART_CONFIG_ACC_MASK > part_index > EXT_CSD_PART_CONFIG_ACC_RPMB

target_part now could be indicating a GP instead of an RPMB leading to failures.

2. part_index <= EXT_CSD_PART_CONFIG_ACC_RPMB

loses the part_index value .

So part_index should be larger than EXT_CSD_PART_CONFIG_ACC_MASK even though the comment indicates it starts at 0?

/**

• struct mmc_rpmb_data - special RPMB device type for these areas
• @dev: the device for the RPMB area
• @chrdev: character device for the RPMB area
• @id: unique device ID number
• @part_index: partition index (0 on first) <---------------------
• @md: parent MMC block device
• @node: list item, so we can put this device on a list

*/ struct mmc_rpmb_data { struct device dev; struct cdev chrdev; int id;

is it just possible that "target_part = idata->rpmb->part_index" just needs to be shifted to avoid issues?

I think the fix to the regression I introduced could perhaps address this as well.

I have no clue how the regression happened really ... heh.

We should probably rename it part_cfg because that is what we store in it, it's assigned from card->part[idx].part_cfg.

Then the id field in mmc_rpmb_data should be deleted along with all the IDA counter code etc and the partition name hardcoded to be "0" as there will never be anything else.

Yours, Linus Walleij

On Wed, Mar 6, 2024 at 4:57 PM Ulf Hansson ulf.hansson@linaro.org wrote:

On Wed, 6 Mar 2024 at 15:38, Linus Walleij linus.walleij@linaro.org wrote:

...

We should probably rename it part_cfg because that is what we store in it, it's assigned from card->part[idx].part_cfg.

Then the id field in mmc_rpmb_data should be deleted along with all the IDA counter code etc and the partition name hardcoded to be "0" as there will never be anything else.

Seems reasonable to me. Are you thinking of sending a cleanup patch on top of $subject patch?

Yes I can do that once this patch is finalized and merged.

Yours, Linus Walleij