Page MenuHomeFreeBSD

PR209468: aacraid fixes to work with newer controller firmware
AcceptedPublic

Authored by yuripv on Sun, Dec 2, 4:04 PM.

Details

Summary

As found in the PR, submitted by Prasad B M <prasad.munirathnam@microsemi.com>.

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

yuripv created this revision.Sun, Dec 2, 4:04 PM
yuripv updated this revision to Diff 51499.Sun, Dec 2, 4:06 PM

Now with full context...

yuripv edited the test plan for this revision. (Show Details)Mon, Dec 3, 11:48 AM
yuripv edited the test plan for this revision. (Show Details)Mon, Dec 3, 11:51 AM
emaste added a subscriber: emaste.Mon, Dec 3, 3:02 PM

IMO go ahead and commit

imp accepted this revision.Mon, Dec 3, 3:33 PM

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.

This revision is now accepted and ready to land.Mon, Dec 3, 3:33 PM
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 added inline comments.Mon, Dec 3, 7:15 PM
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 updated this revision to Diff 51546.Mon, Dec 3, 7:38 PM

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

This revision now requires review to proceed.Mon, Dec 3, 7:38 PM
yuripv marked 3 inline comments as done.Mon, Dec 3, 7:39 PM

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

scottl accepted this revision.Sun, Dec 9, 1:38 AM

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 .

This revision is now accepted and ready to land.Sun, Dec 9, 1:38 AM