Implement initial MMC partitions support for MMCCAM

[kibab]

Summary

For MMC cards, add partitions found on the card as separate disk(9) devices. Don't do anything with RPMB partition for now.

Lots of code is copied almost 1:1 from the mmcsd.c in the old stack, credits Marius Strobl ([e-mail])

Test Plan

Boot BBB:

sdda1: MMC MMC02G 3.10 SN D685362C MFG 02/1997 by 254 0x004e
(sdda1:sdhci_slot1:0:0:0): Partition type 'default', size 983547510784
(sdda1:sdhci_slot1:0:0:0): Partition type 'boot0', size 1048576
(sdda1:sdhci_slot1:0:0:0): Partition type 'boot1', size 1048576
(sdda1:sdhci_slot1:0:0:0): Partition type 'RPMB', size 131072

[marius]

Well, IMO committing the partition identification code only with all the glory details yet to be done is premature, even for head.
Generally: Please adhere to style(9).

[kibab]

Actually implement partitions support

[kibab]

void function shouldn'r return a value, remove sdda_hook_into_geom()

[kibab]

In D12762#264842, @marius wrote:

Well, IMO committing the partition identification code only with all the glory details yet to be done is premature, even for head.
Generally: Please adhere to style(9).

I have now implemented support for attaching partitions as disk(9) and switching them upon access.
Also I adjusted Emacs configuration to do proper indentation according to style(9). Maybe I missed something more, but it should be better now.

Thanks for the review!

[marius]

Generally better, but still quite a few TODOs.
My CAM knowledge is rusty and I've also mainly worked on SIM drivers before so someone vivid in CAM needs to review the CAM-specific parts. The way the commands for switching partitions are inserted/handled may be correct, but at least seems somewhat fishy to me.

sys/cam/mmc/mmc_da.c

118

style(9): "When declaring variables in structures, declare them sorted by use, then by size (largest to smallest), and then in alphabetical order. The first category normally does not apply, but there are exceptions."

178

style(9):

• "The function type should be on a line by itself preceding the function. The opening brace of the function body should be on a line by itself."
• "Insert an empty line if the function has no local variables."
• "Values in return statements should be enclosed in parentheses."

419

style(9): "Space after keywords (if, while, for, return, switch)."

603

According to style(9), variable declarations within functions are accumulated at the beginning of that function, i. e. no C99/11-style variable declaration.

680

Ditto.

708

Using a tab here instead of a single space isn't appropriate, also according to style(9): "When declaring variables in functions declare them sorted by size, then in alphabetical order; multiple ones per line are okay."

750

style(9):

• "Routines returning void * should not have their return values cast to any pointer type" (also note that if casting the return value of malloc(9) would be in order here, style(9) says: "Casts and sizeof's are not followed by a space.").
• "Unary operators do not require spaces, binary operators do."

884

See comment for sdda_get_read_only().

897

That timeout doesn't seem to get correctly handled, i. e. for sdhci(4), apparently it isn't used to set SDHCI_TIMEOUT_CONTROL as appropriate (the MMC layer not supplying bridge drivers with timeouts also is a nasty bug with !MCCAM, though). Also, MMC_CAP_WAIT_WHILE_BUSY and the maximum host timeout need to be taken into account (see mmc_switch() in mmc_subr.c).

903

style(9): "Insert an empty line if the function has no local variables."

925

style(9): "Values in return statements should be enclosed in parentheses."
Also: Missing empty line after variable declaration in a function.

931

Ditto

1087

See comment for sdda_get_read_only().

1118

style(9): ""The function type should be on a line by itself preceding the function."

1119

Missing empty line after variable declaration in a function.

1161

style(9): "Second level indents are four spaces."

1172

That's just the default; for spec_vers >= 6, the actual value should be read from EXT_CSD_GEN_CMD6_TIME.

1318

Probably should be an "else if" as a device can't possibly be (e)MMC chip/card and SD card at the same time.

1333

There's no need to wrap comments on margins shorter than the generic 80 columns maximum ...

1342

... but this line is too long. Also, the comment above this function apparently is at least outdated now.

1350

This should continue using the second level indent of 4 spaces below CAM_DEBUG().

1364

This comment is misleading; eMMC RPMB partitions actually support all I/O operations that would be needed to put file systems onto them. However, apart form most likely requiring unlocking, due to the nature of RPMBs it doesn't seem to make much sense to actually do so. Instead, it would appear to be more appropriate to create a userland tool tailored to the actual use case or wrap around the RPMB partition support of e. g. mmc-utils.

1396

Hrm, why is that code block left comment out (apparently, the sdda_hook_into_geom() had it in).

1405

Side-note: Prior to UHS-II, the maximum transfer size for SDHCI is limited by the 16-bit block count register (that maximum may even shrink further depending on the re-tuning mode a particular SDHCI controller implements, though). So for !MMCCAM, shdci(4) correctly hardcodes 65535 for MMCBR_IVAR_MAX_DATA in the !re-tuning case. However, in case of MMCCAM, shdci(4) currently incorrectly sets cpi->maxio to MAXPHYS for XPT_PATH_INQ.

1412

Currently, MMCCAM doesn't seem to invoke a MMC_ERASE command anywhere so it appears that DISKFLAG_CANDELETE/BIO_DELETE isn't actually supported so far. Also, if it actually does, it seems that part->disk->d_delmaxsize would be needed to be set somewhere.

1452

Excessive newline.

1504

Though, actually doing something with extended user data area information e. g. by registering such partitions with geom_flashmap(4) is still missing.

1542

Should use 4 space second level indent relative to sdda_add_part().

1546

Ditto

1586

Wrapping should make use of the 80 columns margin.

1590

Should use a single space after the type instead of a tab.

1594

What guarantees that the kernel copy of the EXT_CSD - specifically the upper bits of EXT_CSD_PART_CONFIG - isn't outdated as userland has changed the content? For mmcsd(4), mmcsd_ioctl_cmd() rereads EXT_CSD as necessary as that IOCTL handler is the only way userland can alter EXT_CSD (specifically, setting the partition to boot from in EXT_CSD_PART_CONFIG is a likely change). However, with CAM an additional way for userland to fiddle with these bits is via pass(4). So there appears to be a hook missing which rereads EXT_CSD also on accesses via the latter.

1625

According to style(9), two spaces are required after a period within a comment.

1626

According to style(9), variable declarations within functions are accumulated at the beginning of that function.

1640

Just as with the remainder of EXT_CSD, userland may change the currently selected partition so the kernel must not trust its last accessed partition to still be the active one if there was a write access to EXT_CSD by userland in between (which is something the current in-tree mmcsd_ioctl_cmd() also doesn't handle correctly, though). Apart from that, unnecessary partition switches should be avoiding of course.
Also, how does MMCCAM handle suspend/resume? With !MMCCAM, mmc(4) and mmcsd(4) re-attach on resume so they can't possibly rely on state which the device might have lost while being suspended. For MMCCAM, sdhci(4) suggests that only a re-scan is triggered so does softc->part_curr need to be invalidated on resume/re-scan?

1641

Line exceeds 80 columns maximum.

1767

According to style(9), variable declarations within functions are accumulated at the beginning of that function, but probably better yet, just use mmcio->cmd.resp[0] directly below so compilers won't complain about an unused/written-only variable if CAM_DEBUG is defined no nothing.

1784

Hrm, how does that guarantee that in case the switching fails, the outstanding deletes/reads/writes in the I/O queue also fail and won't go to the wrong partition?

[kibab]

Address review comments

[kibab]

Hi Marius, thank you very much for such a verbose review!
I have addressed most of your comments. As for the remaining ones -- they require some more work which I'd like to do in the upcoming changes. As all my work should be reviewed by Warner before committing, I'd like to try to keep the diff sizes under certain limit.

sys/cam/mmc/mmc_da.c

1350

It was indented like that because this is a continuation inside the braces. But I will align everything with 4 spaces.

1412

Fair point.

1594

Everything that is happening through pass(4) is irrelevant here, the instance of sdda is not processing any pass requests. The user is advised that fiddling with camcontrol can have unwanted side effects. The only way to recover after direct manipulations with pass(4) is to initiate rescan and reattach.

1640

With regards to changes through pass(4) see my comment above.
"Unnecessary partitions switch" I'm avoiding by first checking if there is something to do for the current partition.
Suspend / resume is entirely out of the question for now. I even don't know how to test that all... Or does NUC support suspend/resume?

1784

The index of active partition is not changed in this case. So when the next BIO is processed and that BIO targets new partition, the code will attempt to switch the partition again.
Although it might be better to just fail all outstanding BIO requests here instead. Not sure.

[imp]

This looks OK-ish. There's still a fair amount of cut and paste going on, but that's no different than other drivers. There's a few things we can cleanup, but maybe in a separate commit.
After the updates for Marius' suggestions, we're close enough to style(9) to be ready to go.
I'm clicking Accept, but will feel better of Marius double checks my belief.

sys/cam/mmc/mmc_da.c

519

Do you have on your list to move to the cam I/O scheduler?

1158

Is this really a panic? Is it a never can happen / programming error detection class event?

1360–1362

This list seems overly long. SEND_BDR and BUS_RESET I thought were purely SCSI.... I don't see where they can be sent...

1404–1407

Maybe I should create a wrapper for XPT_PATH_INQ... I'll talk to scott about it... But no action item...

[kibab]

sys/cam/mmc/mmc_da.c

519

Yes, see https://github.com/kibab/freebsd/issues/6
Although this has lower priority than reaching feature parity with the old stack...

1158

This should never happen, at least for SDHCI-based controllers. For non-SDHCI panic is appropriate because reply to GET_TRAN_SETTINGS absolutely has to be implemented, but there is no way to enforce it at compile-time...

1360–1362

Yeah, I checked the sources for possible mentions of SEND_BDR and BUS_RESET and it seems they are sent only in SCSI-specific code. I will remove this and related case blocks in sddaasync()

1404–1407

What would your new wrapper do?

[kibab]

Address review comments

[marius]

Again better; at this point, I'm mainly concerned about:

• Handling of the timeout provided in mmc_switch_fill_mmcio(); partly this needs to happen in sdhci(4), i. e. SDHCI_TIMEOUT_CONTROL needs to be set accordingly, but a counterpart to mmc_switch() of mmc_subr.c should live in CAM.
• Handling of suspend/resume in sdda(4) but also already starting in sdhci(4) in the MMCCAM case.
• Usage of cpi.maxio for disk->d_maxsize with the former set to an incorrect value by sdhci(4) (which obviously is an sdhci(4) bug, though).

sys/cam/mmc/mmc_da.c

1640

You can send suspend/resume events to e. g. sdhci(4) instances via devctl(8), which should even work on ARM (but doing so with the system booted from an SD card probably isn't a good idea for starters).

[kibab]

Marius,
thanks for your review.
I have created separate issues for each of the point you've mentioned: https://github.com/kibab/freebsd/projects/1
Seems none of those are directly related to the scope of this particular change. Do you think it's fine to commit this code so that partition functionality is in -HEAD?

[marius]

Well, to get en par with the eMMC partition functionality in mmc(4), there still needs to be code added which actually does something with the extended user data area information and a way to create partitions. As for the later, adding such support to camcontrol(8) probably would be the more natural thing to do with CAM. But given that partitioning eMMC devices is a one-time-operation, I'd strongly suggest to just also add a Linux-compatible MMC IOCTL interface so mmc-utils can be continued to be used unless you want to risk to brick a couple of eMMC chips (besides, there are tutorials how to partition with mmc-utils). I'm okay with this patch getting committed as it currently is, though.
Btw., at least currently I don't seem to be able to access your list of issues; even after multiple attempts to reload that page and besides waiting for several minutes, all Firefox is displaying are the spinning cats waiting/"progress" indicators where the lists should go ...