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

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

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

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

Ditto.

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

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

See comment for sdda_get_read_only().

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

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

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

Ditto

See comment for sdda_get_read_only().

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

Missing empty line after variable declaration in a function.

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

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

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

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

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

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

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.

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

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.

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.

Excessive newline.

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

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

Ditto

Wrapping should make use of the 80 columns margin.

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

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.

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

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

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?

Line exceeds 80 columns maximum.

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.

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?

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

Fair point.

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.

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?

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.

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

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

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()

What would your new wrapper do?

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