Page MenuHomeFreeBSD

Fix mpr driver endianness

Authored by on Jul 23 2020, 12:18 PM.
Referenced Files
Unknown Object (File)
Thu, Jan 12, 8:17 AM
Unknown Object (File)
Mon, Jan 2, 2:20 AM
Unknown Object (File)
Dec 17 2022, 5:09 AM
Unknown Object (File)
Dec 2 2022, 10:24 AM



This patch fix endiannes issues in the mpr driver when functioning in a big endian platform. The changes were tested with a SAS9300-8i card chip LSISAS3008 (pci vendor=0x1000 device=0x0097).
The mpr driver was also added to GENERIC64 conf for ready usage on future.

It was also tested on a experimental ppc64le kernel.

Test Plan

Compile and install kernel with changes and see if tools like 'geom disk list' and 'camcontrol inquiry' are able to fetch disk information.

Diff Detail

rS FreeBSD src repository - subversion
Lint Passed
No Test Coverage
Build Status
Buildable 37382
Build 34271: arc lint + arc unit

Event Timeline


Why data16[0] is not converted with le16toh() while the other elements of data16[] are?


This part is a bit confusing.

mpr_regread() uses bus_space_read_4() that, on big-endian (BE) platforms, already converts from little-endian (LE) to big-endian, right?
And constant values, such as MPI2_DOORBELL_DATA_MASK, are always expected to be in host endian.
So, it seems to me that this le16toh() call is actually converting a BE value to LE and not the opposite.

IIUC, adjust_iocfacts_endianness() is the part that really fixes the endianess of the reply, with several leXXtoh() calls, as it has the knowledge of the fields present in the reply. And the comment there tell us that these le16toh() calls here are only needed to preserve U8 fields in their original offsets.

So my suggestion here is to change all data16[] conversions to be16toh(), to make it clear what is happening.


This function swaps U16 and U32 fields.


Indentation is wrong.

Each level of indentation must be a (non-expanded) tab. 4 extra spaces should be used when continuing a line of code.
This comment also applies to the rest of the changes.

See style(9).


Why do you need two calls to le32toh()here, with masks, instead of only one with no mask?


It looks like the first 16 bits of MPI2_DEFAULT_REPLY are not used


It is a bit confusing, yes. If not swapping values here, adjust_iocfacts_endianness would have to adjust 8 bit fields with adjacent fields, and I thought it would be even more confusing. The suggestion of be16toh is a good one, thanks. edited the test plan for this revision. (Show Details)

Comments and indentation.

Nice. I just have a few more nitpicks.


Ok, but I would use le16toh() here too, just in case the first 16 bits are used someday and for consistency with the other data16 elements that are all being converted.


You need 2 more spaces here.


It would be better to put this comment before the first use of le16toh() data16.


You should use 2 tabs here.


You should use 2 tabs here.

This revision is now accepted and ready to land.Aug 3 2020, 7:01 PM

The SMID doesn’t need to be byte swapped. It's a host-private cookie that is opaque to the controller. Be sure to also not byte-swap the retrieval of the SMID on completion.


As stated previously, this is wrong. The le16toh() call should wrap just the return from mpr_regread(), and not include the bitwise & operation and the MPI2_DOORBELL_DATA_MASK macro.


Yeah, as above, I have no idea how this can work on a BE platform.


Probably makes sense to pre-swap these values. Not necessary for this review, however.


I put this here because the original code assumes that SMID is LE, lines 2555 and 2597 for example. edited the summary of this revision. (Show Details) edited the test plan for this revision. (Show Details)

Review fixes.

This revision now requires review to proceed.Oct 30 2020, 5:58 PM

Looks good to me. Did someone test it for regression on LE platform?

Looks good to me. Did someone test it for regression on LE platform?

Nevermind, I see this was tested on both powerpc64 BE and LE platforms.

This revision is now accepted and ready to land.Nov 12 2020, 2:20 PM

Someone any additional comment on this?

Add mpr driver into GENERIC64LE configuration.

This revision now requires review to proceed.Feb 25 2021, 7:56 PM

Also reviewed by Sreekanth Reddy (by e-mail)

This revision is now accepted and ready to land.Mar 2 2021, 11:02 AM