PR209468: aacraid fixes to work with newer controller firmware

[yuripv]

Summary

As found in the PR, submitted by Prasad B M <[e-mail]>.

Test Plan

For the moment, tested on older ASR6805, not showing any issues.

Reported on stable@ to fix issues with ASR8805.

More testing still pending.

[yuripv]

Now with full context...

[emaste]

IMO go ahead and commit

[imp]

The code seems fine, other than the weird msi_tupelo which randomly seems to be the same as msi_enabled, but only some of the time. A comment is needed to explain why the oddity.

sys/dev/aacraid/aacraid_var.h

488

This needs a comment. It kinda sorta seems like it duplicates msi_enabled, but it kinda doesn't in all cases. It's weird and after studying the code for a bit, I'm still at a loss to understand why it's needed (apart from "to work" which is unsatisfying an answer). msi_enabled appears to be overloaded for a few other things, but it's unclear why from reading the code w/o firmware interface docs.

[scott.benesh_microsemi.com]

sys/dev/aacraid/aacraid_var.h

488

According to Prasad:

"msi_tupelo" is used for ARC series 6 controller which initially did not support MSI interrupt, later support was added with only one MSI interrupt. Basically it means it supports only MSI interrupt.

where as "msi_enabled" indicates that it supports both MSI and MSI-X interrupt based on the System Config. this is usually used for Series 7 & 8 ARC controller.

[imp]

sys/dev/aacraid/aacraid_var.h

488

So adding a comment /* Series 6 support for single MSI interrupt. */ after msi_tupelo would answer my concern here nicely I think. thanks for the explanation.

[yuripv]

Add the comment for msi_tupelo (and cleanup whitespace issues introduced by the patch).

[yuripv]

With the comment updated, should I go ahead and commit this? Or we should still wait for actual reviews from @scottl and @mav?

[scottl]

I'm marking this as accepted despite my individual comments.

sys/dev/aacraid/aacraid.c

3730

This ultimately gets passed to cam_path_inquiry->initiator_id which is an unsigned int. It's an almost irrelevant semantic difference in this case but could cause a compiler warning in the future. It's a bug in CAM that there isn't a suitable constant available to express this correctly. For now I'd just set it to businfo.TargetsPerBus+1, unless that evaluates into an overflow.

sys/dev/aacraid/aacraid_var.h

488

Would it make better sense to always treat MSI as having a single vector, rename msi_enabled as msix_enabled, and rename msi_tupelo to msi_enabled? You could then flag the Gen6 behavior as a quirk in the device table in aacraid_pci.c .

[evfbsd_yandex.ru]

Hi @yuripv, are there plans to commit this?

[yuripv]

In D18408#400188, @evfbsd_yandex.ru wrote:

Hi @yuripv, are there plans to commit this?

Sorry for the delay, just got the system with ASR6805 back so I can test the changes @scottl is asking for (not that they are big, just to be sure).

[evfbsd_yandex.ru]

Near release 11.3, is there a chance for this patch to enter release?

[admin_maxnet.ua]

Hi!

Is there any news on this patch, is there a chance that it will appear in the 11.3 or 12.1 version?

thank you