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 (marius@FreeBSD.org)
Details
Details
- Reviewers
imp marius - Commits
- rS334065: Implement initial MMC partitions support for MMCCAM.
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
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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).
Comment Actions
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!
Comment Actions
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 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | style(9):
|
| 419 ↗ | (On Diff #34415) | style(9): "Space after keywords (if, while, for, return, switch)." |
| 603 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | Ditto. |
| 708 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | style(9):
|
| 884 ↗ | (On Diff #34415) | See comment for sdda_get_read_only(). |
| 897 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | style(9): "Insert an empty line if the function has no local variables." |
| 925 ↗ | (On Diff #34415) | style(9): "Values in return statements should be enclosed in parentheses." |
| 931 ↗ | (On Diff #34415) | Ditto |
| 1087 ↗ | (On Diff #34415) | See comment for sdda_get_read_only(). |
| 1118 ↗ | (On Diff #34415) | style(9): ""The function type should be on a line by itself preceding the function." |
| 1119 ↗ | (On Diff #34415) | Missing empty line after variable declaration in a function. |
| 1161 ↗ | (On Diff #34415) | style(9): "Second level indents are four spaces." |
| 1172 ↗ | (On Diff #34415) | That's just the default; for spec_vers >= 6, the actual value should be read from EXT_CSD_GEN_CMD6_TIME. |
| 1318 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | There's no need to wrap comments on margins shorter than the generic 80 columns maximum ... |
| 1342 ↗ | (On Diff #34415) | ... but this line is too long. Also, the comment above this function apparently is at least outdated now. |
| 1350 ↗ | (On Diff #34415) | This should continue using the second level indent of 4 spaces below CAM_DEBUG(). |
| 1364 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | Hrm, why is that code block left comment out (apparently, the sdda_hook_into_geom() had it in). |
| 1405 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | Excessive newline. |
| 1504 ↗ | (On Diff #34415) | Though, actually doing something with extended user data area information e. g. by registering such partitions with geom_flashmap(4) is still missing. |
| 1542 ↗ | (On Diff #34415) | Should use 4 space second level indent relative to sdda_add_part(). |
| 1546 ↗ | (On Diff #34415) | Ditto |
| 1586 ↗ | (On Diff #34415) | Wrapping should make use of the 80 columns margin. |
| 1590 ↗ | (On Diff #34415) | Should use a single space after the type instead of a tab. |
| 1594 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | According to style(9), two spaces are required after a period within a comment. |
| 1626 ↗ | (On Diff #34415) | According to style(9), variable declarations within functions are accumulated at the beginning of that function. |
| 1640 ↗ | (On Diff #34415) | 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. |
| 1641 ↗ | (On Diff #34415) | Line exceeds 80 columns maximum. |
| 1767 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | 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? |
Comment Actions
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 ↗ | (On Diff #34415) | It was indented like that because this is a continuation inside the braces. But I will align everything with 4 spaces. |
| 1412 ↗ | (On Diff #34415) | Fair point. |
| 1594 ↗ | (On Diff #34415) | 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 ↗ | (On Diff #34415) | With regards to changes through pass(4) see my comment above. |
| 1784 ↗ | (On Diff #34415) | 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. |
Comment Actions
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 ↗ | (On Diff #35168) | Do you have on your list to move to the cam I/O scheduler? |
| 1158 ↗ | (On Diff #35168) | Is this really a panic? Is it a never can happen / programming error detection class event? |
| 1360–1362 ↗ | (On Diff #35168) | 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 ↗ | (On Diff #35168) | Maybe I should create a wrapper for XPT_PATH_INQ... I'll talk to scott about it... But no action item... |
| sys/cam/mmc/mmc_da.c | ||
|---|---|---|
| 519 ↗ | (On Diff #35168) | Yes, see https://github.com/kibab/freebsd/issues/6 |
| 1158 ↗ | (On Diff #35168) | 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 ↗ | (On Diff #35168) | 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 ↗ | (On Diff #35168) | What would your new wrapper do? |
Comment Actions
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 ↗ | (On Diff #34415) | 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). |
Comment Actions
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?
Comment Actions
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 ...