Page MenuHomeFreeBSD

Add new fields to mmc_data in preparation to SDIO CMD53 block mode support
ClosedPublic

Authored by kibab on Apr 1 2019, 7:25 PM.

Details

Summary

SDIO command CMD53 (IO_RW_EXTENDED) allows data transfers using blocks of 1-2048 bytes,
with a maximum of 511 blocks per request.
Extend mmc_data structure to properly describe such requests,
and initialize the new fields in kernel and userland consumers.

No actual driver changes happen yet, these will follow in the separate changes.
This change breaks kernel ABI for CAM calls, so camcontrol should be recompiled.

Test Plan

No functional changes.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kibab created this revision.Apr 1 2019, 7:25 PM
kibab retitled this revision from Add new fields to mmc_data in preparation to SDIO CMD53 block mode support SDIO command CMD53 (IO_RW_EXTENDED) allows data transfers using blocks of 1-2048 bytes, with a maximum of 511 blocks per request. Extend mmc_data structure to properly... to Add new fields to mmc_data in preparation to SDIO CMD53 block mode support.Apr 1 2019, 8:03 PM
kibab edited the summary of this revision. (Show Details)
kibab set the repository for this revision to rS FreeBSD src repository.
kibab added a project: ARM.
bz requested changes to this revision.Apr 1 2019, 8:59 PM

This is missing files I think. The mmcreg.h structure changes are missing from this one. This won't compile.

This revision now requires changes to proceed.Apr 1 2019, 8:59 PM
kibab added a comment.Apr 1 2019, 9:04 PM
In D19779#424220, @bz wrote:

This is missing files I think. The mmcreg.h structure changes are missing from this one. This won't compile.

Sorry, something has gone wrong with my tree. I will reupload.

imp added a comment.Apr 1 2019, 10:23 PM
In D19779#424220, @bz wrote:

This is missing files I think. The mmcreg.h structure changes are missing from this one. This won't compile.

Sorry, something has gone wrong with my tree. I will reupload.

IF you jhust added the block count and block size to the end, there's no need to rebuild camcontrol...

kibab updated this revision to Diff 55706.Apr 1 2019, 10:56 PM

Added (hopefully) missing files.

imp added inline comments.Apr 1 2019, 10:59 PM
sys/dev/mmc/mmcreg.h
193–194 ↗(On Diff #55706)

It looks like if we add these to the end, we don't break binary compat...
But does camcontrol even know about mmc_data yet?

kibab added a comment.Apr 1 2019, 11:00 PM
In D19779#424273, @imp wrote:
In D19779#424220, @bz wrote:

This is missing files I think. The mmcreg.h structure changes are missing from this one. This won't compile.

Sorry, something has gone wrong with my tree. I will reupload.

IF you jhust added the block count and block size to the end, there's no need to rebuild camcontrol...

As you will see, these fields were added not to the end, but rather to the place where I felt most appropriate :-) Since we are at -CURRENT and don't plan to MFC, I thought this should be fine?

kibab marked an inline comment as done.Apr 1 2019, 11:03 PM
kibab added inline comments.
sys/dev/mmc/mmcreg.h
193–194 ↗(On Diff #55706)

Well, if you don't zero those fields when passing structure to the kernel, these fields end up being filled with the random garbage, and this changes the behavior of the SDHC driver because now it thinks that the upper layer requested the custom block size.
And this happens regardless of the position of these fields in the struct.
camcontrol does know about mmc_data, see the code in this change:-)

kibab marked an inline comment as done.Apr 2 2019, 3:30 PM
emaste added a subscriber: emaste.Apr 2 2019, 3:34 PM
bz added inline comments.Apr 2 2019, 4:12 PM
sbin/camcontrol/camcontrol.c
7795 ↗(On Diff #55706)

Well, given there is a memset(,0,) above it you don't need the extra assignments, do you?

sys/cam/mmc/mmc_da.c
794 ↗(On Diff #55706)

On a side-note; that malloc can fail...

1812 ↗(On Diff #55706)

The softc mmcdata is allocated with M_ZERO; if it is not used anywhere else than here block_count and block_size are never touched so wouldn't need to be set to 0.
In general, if needed, I'd prefer an memset(,0,) on the structure as that way it'll not repeat the problem with other struct fields in the future?

sys/dev/mmc/mmcreg.h
578 ↗(On Diff #55706)

These all are independent on the change kind-of and only needed for using the change later; really nothing to do with block_count and block_size. Can you commit them on their own please?

imp added inline comments.Apr 2 2019, 4:28 PM
sys/dev/mmc/mmcreg.h
193–194 ↗(On Diff #55706)

But we bzero the CCB, which is larger than mmc_data so it won't be garbage...
And you could also add a flag MMC_DATA_BLOCK_SIZE that needs to be set for these values to be used.

kibab updated this revision to Diff 55844.Apr 5 2019, 4:48 PM
kibab marked 4 inline comments as done.

Addressed comments

kibab updated this revision to Diff 55845.Apr 5 2019, 4:52 PM
kibab marked an inline comment as done.

Killed CMD53-related changes, leaving only field changes.

kibab added a comment.Apr 5 2019, 5:01 PM

Updated; please take a look.

sbin/camcontrol/camcontrol.c
7795 ↗(On Diff #55706)

that memset() is for mmc_data variable, not for mmc_d. But I will better do another memset(,0,) for mmc_d as well.

sys/cam/mmc/mmc_da.c
794 ↗(On Diff #55706)

Added a check for that.

1812 ↗(On Diff #55706)

That mmcdata ends up being used in each sddastart(), so we _do_ need to initialize the fields. But you are right, I'd rather add memset(,0,).

sys/dev/mmc/mmcreg.h
193–194 ↗(On Diff #55706)

CCB contains struct mmc_command and mmc_command contains a _pointer_ to mmc_data, not the actual mmc_data. So it will be garbaged unless initialized properly.

578 ↗(On Diff #55706)

Done

bz accepted this revision.Apr 6 2019, 8:51 PM

OK, this seems fine to me.

This revision is now accepted and ready to land.Apr 6 2019, 8:51 PM
kibab updated this revision to Diff 55901.Apr 7 2019, 11:47 AM

Reupload the change.

This revision now requires review to proceed.Apr 7 2019, 11:47 AM
kibab updated this revision to Diff 55902.Apr 7 2019, 11:53 AM

Try again

imp added inline comments.Apr 8 2019, 7:56 PM
sys/cam/mmc/mmc_da.c
794 ↗(On Diff #55902)

In theory, we should retry at some other level... In practice, this will never happen and it's likely super hard to do that retry right anyway with any kind of meaningful testing... Other periph drivers do this so it's fine.

sys/dev/mmc/mmcreg.h
202 ↗(On Diff #55902)

so you have this flag, but don't use it.
And you've not moved the new elements to the end.

kibab marked an inline comment as done.Apr 10 2019, 7:41 PM
kibab added inline comments.
sys/dev/mmc/mmcreg.h
202 ↗(On Diff #55902)

Currently there is no driver that can use this flag. mmc_da is not one of them.

Moving elements -- ack.

kibab updated this revision to Diff 56069.Apr 10 2019, 7:41 PM
kibab marked an inline comment as done.
  • Move new fields to the end
imp accepted this revision.Apr 10 2019, 7:43 PM
This revision is now accepted and ready to land.Apr 10 2019, 7:43 PM
This revision was automatically updated to reflect the committed changes.