Hi john,

Hi John,

its been long the patches not reviewed so please review the patch and provide your comments.

its been long the patches not reviewed so please review the patch and provide your comments.

its been long the patches not reviewed so please review the patch and provide your comments.

its been long the patch not reviewed so please review the patch and provide your comments.

its been long the patches not reviewed so please review the patch and provide your comments.

Hi John,
I wanted to check if there are any updates on the suggestion I had asked.

In D52163#1195416, @jhb wrote:

I need to think about this. I don't really want to have the PCI bus directly reading the config registers from the parent bridge. It's also true that you want to not release the bus until all VFs using that bus have been torn down, so you really kind of want a refcounted bus_alloc_resource here (in essence). It might be that just switching from RF_ACTIVE to RF_SHAREABLE when calling bus_alloc_resource is sufficient however.

In D52116#1190912, @imp wrote:

So how it's used now is fine, but the min_t and max_t macros coerce their arguments to the given type before comparison. This can cause weird results then int values exceed the size (in this case U8). Since the only thing like this is a #define, I think it's OK for now. Just be aware.

In D52148#1191111, @imp wrote:

I think that while our driver may be up to date, our man page describing what's supported may be lagging a little, especially for OEMs. Is that something you can look into updating?

In D52115#1190909, @imp wrote:

I'm assuming the regen is correct. I've not checked this against the latest data sheet.

In D50134#1145957, @imp wrote:

I think this is good...

Hi Chs,

In D49749#1135942, @chs wrote:

Ok, I didn't realize that "task management" always meant "target reset" here. Not waiting for in-progress commands makes sense in this context. Could you add a comment to the code explaining this?

Another thing that I noticed while looking at this patch was that all the other places that decrement block_io do it in this style:

if (mpi3mr_atomic_read(&tgtdev->block_io) > 0)
        mpi3mr_atomic_dec(&tgtdev->block_io);

In D49749#1135018, @chs wrote:

This change blocks new I/Os from being submitted while a task management command is in progress, but wouldn't you need to also wait for any in-progress I/Os to complete before submitting the task management command? I think you would need to wait for those if your goal is to avoid having the firmware processing I/O commands and task management commands together (which is what this change appears to be trying to do).

In D49743#1134834, @imp wrote:

I have no way to verify all this easily ( I do have the tech docs, but that's a lot to plow through).
These changes look fine for what they do, though, so I'll trust the process and approve.

In D49745#1134837, @imp wrote:

Might be better to use the current latest reversion number in the commit message since there will be newer ones in the future and that would make it easier to use the commit log to figure out when this came in.

In D49746#1134846, @imp wrote:

This seems OK, but I can't check that the details done here matches the firmware running on the card.
I'll trust the vendor, though, to get this right. The FreeBSD details look good though.

In D49754#1134788, @imp wrote:

Should you include this for aarch64? Or is that not supported by broadcomm yet?

Thank you check for reviewing and suggesting the required changes.

• Added compile-with "${LINUXKPI_C}" for mpi3mr_cam.c
• Removed the mpi3mr entry from "sys/i386/conf/GENERIC"

In D49754#1134464, @chs wrote:

I tried building with the new patch and found more problems:

• mpi3mr_cam.c uses some linux kernel interfaces and so that entry in sys/conf/files for that source file needs to have this added at the end of the line: compile-with "${LINUXKPI_C}"
• there are a bunch of compile errors when building for i386 (aka. 32-bit x86). most of the errors are just mismatches between printf format strings and the corresponding values, and those would be easy to fix, but there are some that might be harder to fix. mpi3mr.h defines mpi3mr_regread64() and mpi3mr_regwrite64() that use bus_space_read_8() and bus_space_write_8() to access 64-bit registers in the card, but 32-bit x86 hardware does not have the ability to do 64-bit reads and writes. if these accesses really have to be 64-bit accesses then this card just can't work on 32-bit x86. but if the 64 bits can be read or written as two 32-bit values then the driver could do that instead of one 64-bit read or write. or, if you don't care about 32-bit x86 then the simplest thing would just be to leave out this change to sys/i386/conf/GENERIC.

In D49754#1134431, @chs wrote:

in addition the changes you have here already, you'll also need to add the same line that you added to sys/conf/NOTES to sys/amd64/conf/GENERIC and sys/i386/conf/GENERIC.

Below changes are suggested by Chuck.

Thank you all for reviewing the code.

• I missed updating the SPATH for CFLAGS in the Makefile. This has been corrected in the current revision.

• Updated the Makefile. Renamed the _spath varaible to SPATH
• reordered the header files as per the style.9 man page standard
• removed the manual #define of bool flags.

Hi Markj

• Accommodated all the review feedback

In D45011#1028836, @imp wrote:

The build is good, modulo one quibble.
The rest looks OK, but is really too large for me to review.

In D45011#1028825, @markj wrote:

Could you please add a manual page for the new drivers, or extend the existing share/man/man4/bnxt.4?

Thanks for the feedback! I'll create a new man page for the new RoCE drivers and submit it accordingly.

In D45009#1028830, @imp wrote:

ah, mechanically generated code.

In D45007#1028826, @imp wrote:

This looks straight forward enough that I'm happy ...

Thanks for the review! I will update the Makefile.

In D45005#1028822, @imp wrote:

The build stuff looks good.

Thank you. I am glad that the build changes are acceptable.

I can't speak to the hardware.

Thanks for the input! Regarding the hardware, I've tested the patch on relevant hardware and haven't encountered any issues. While I don't currently have documentation detailing the testing process, I can assure you that thorough testing has been conducted. If there's any specific information you need or if further testing is required on certain hardware configurations, please let me know, and I'll be happy to provide it

The extraneous (white space) changes are relatively low for vendor updates and don't get too much .

Thanks for your input! We'll address the whitespace in a separate cleanup patch after this patchset accepted.

In D44494#1014836, @imp wrote:

Change looks good, but you'll want to edit the 'subject' of the commit message to be < 70 characters