Fix mprutil endianness
ClosedPublic
Actions

Authored by afscoelho_gmail.com on Aug 12 2020, 1:39 PM.

Details

Reviewers

alfredo
luporl
jhibbits
scottl

Commits

rGa027f2ab422c: mprutil: add big endian support
rG7d45bf699dc6: mprutil: add big endian support
rGfc9780fd41e7: mprutil: add big endian support

Summary

This patch fix endianness issues in the mprutil utility. Except for flash operations (firmware/bios save/update), other query operations do work when functioning with a mpr driver also fixed with proper endianness conversions (https://reviews.freebsd.org/D25785).

Flash operations are not fixed by this patch yet. Tested firmware version ( 16.00.01.00) do not respond to flash operations like for example MPI2_FUNCTION_FW_UPLOAD, giving a timeout.

Test Plan

Use the patched mpr driver version (https://reviews.freebsd.org/D25785).
Compile and execute mprutil, and execute for example:

$ mprutil show iocfacts

   MsgVersion: 2.5
    MsgLength: 17
     Function: 0x3
HeaderVersion: 50,00
    IOCNumber: 0

...

If you try to execute a flash operation, an error like the one bellow should appear :

mprutil flash save firmware firm.bin

mpr0: mpr_user_command: unsupported parameter or unsupported function in request (function = 0x12)
mprutil: Fail to save firmware

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 32896
Build 30296: arc lint + arc unit

Event Timeline

afscoelho_gmail.com created this revision.Aug 12 2020, 1:39 PM

Herald added a subscriber: bdragon. · View Herald TranscriptAug 12 2020, 1:39 PM

afscoelho_gmail.com requested review of this revision.Aug 12 2020, 1:39 PM

Harbormaster completed remote builds in B32896: Diff 75645.Aug 12 2020, 1:39 PM

Looks good to me.

This revision is now accepted and ready to land.Aug 20 2020, 11:45 AM

Looks good to me. @scottl , would you like to take a look on this patch as well?

alfredo added a reviewer: scottl.Feb 9 2021, 1:29 PM

@afscoelho_gmail.com, would you mind adding TODO section in usr.sbin/mpsutil/mpsutil.8 regarding flash operations not working on BE (timeout) ?
You can use D28475 as reference. It can be in a different review linked to this one (child revision).

Thank you for doing this. My main suggestion is to fix mps_read_extended_config_page so it returns IOCStatus already byte-swapped, which will eliminate a lot of the other diffs. I also recommend looking at mps_cmd.c and fixing code that is under #ifdef USE_MPT_IOCTLS as well as not

usr.sbin/mpsutil/mps_cmd.c
401	IOCStatus should be byte-swapped here and stored for return to the caller so it doesn't have to be byte swapped by the caller
454	IOCStatus should be byte-swapped here and stored for return to the caller so it doesn't have to be byte swapped by the caller
usr.sbin/mpsutil/mps_show.c
141	This, and many others, shouldn't need to be byte-swapped if `mps_read_extended_config_page` does the swapping itself.

scottl requested changes to this revision.Feb 9 2021, 2:52 PM

This revision now requires changes to proceed.Feb 9 2021, 2:52 PM

Fixes after review. Make mps_read_extended_config_page return IOCStatus already byte-swapped. Other missing swaps fixed. Updated man page with TODO about flash operations not working on BE.

Harbormaster completed remote builds in B37028: Diff 83943.Feb 15 2021, 6:09 PM

afscoelho_gmail.com marked 3 inline comments as done.Feb 15 2021, 6:10 PM

Also reviewed by Sreekanth Reddy (by e-mail)

@scottl are you ok with latest changes?

luporl accepted this revision.Apr 20 2021, 7:22 PM

This revision was not accepted when it landed; it landed in state Needs Review.Apr 20 2021, 7:23 PM

Closed by commit rGfc9780fd41e7: mprutil: add big endian support (authored by alfredo). · Explain Why

This revision was automatically updated to reflect the committed changes.

alfredo added a commit: rGfc9780fd41e7: mprutil: add big endian support.

alfredo added a commit: rG7d45bf699dc6: mprutil: add big endian support.May 7 2021, 2:39 AM

mav added a commit: rGa027f2ab422c: mprutil: add big endian support.Dec 1 2021, 6:59 PM

Herald added a subscriber: imp. · View Herald TranscriptDec 1 2021, 6:59 PM