Fix mpr driver endianness

[afscoelho_gmail.com]

Summary

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.

[luporl]

sys/dev/mpr/mpr.c

1058

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

1066

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.

1158

This function swaps U16 and U32 fields.

1163–1183

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

1170–1171

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

[afscoelho_gmail.com]

sys/dev/mpr/mpr.c

1058

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

1066

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.

[afscoelho_gmail.com]

Comments and indentation.

[luporl]

Nice. I just have a few more nitpicks.

sys/dev/mpr/mpr.c

1058

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.

1066

You need 2 more spaces here.

1092–1096

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

2770

You should use 2 tabs here.

2825

You should use 2 tabs here.

[afscoelho_gmail.com]

Review comments fixes.

[scottl]

sys/dev/mpr/mpr.c

1576

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.

[scottl]

sys/dev/mpr/mpr.c

1059

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.

1092

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

1147

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

[afscoelho_gmail.com]

sys/dev/mpr/mpr.c

1576

I put this here because the original code assumes that SMID is LE, lines 2555 and 2597 for example.

[afscoelho_gmail.com]

Review fixes.

[alfredo]

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

[alfredo]

In D25785#606744, @alfredo wrote:

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.

[afscoelho_gmail.com]

Someone any additional comment on this?

[afscoelho_gmail.com]

Add mpr driver into GENERIC64LE configuration.

[alfredo]

Also reviewed by Sreekanth Reddy (by e-mail)