Page MenuHomeFreeBSD

PR209468: aacraid fixes to work with newer controller firmware

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



As found in the PR, submitted by Prasad B M <>.

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

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

Now with full context...

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

IMO go ahead and commit

imp accepted this revision.Dec 3 2018, 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.

488 ↗(On Diff #51499)

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.Dec 3 2018, 3:33 PM
488 ↗(On Diff #51499)

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.Dec 3 2018, 7:15 PM
488 ↗(On Diff #51499)

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.Dec 3 2018, 7:38 PM

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

This revision now requires review to proceed.Dec 3 2018, 7:38 PM
yuripv marked 3 inline comments as done.Dec 3 2018, 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.Dec 9 2018, 1:38 AM

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

3730 ↗(On Diff #51546)

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.

488 ↗(On Diff #51499)

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.Dec 9 2018, 1:38 AM

Hi @yuripv, are there plans to commit this?

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

Near release 11.3, is there a chance for this patch to enter release? added a comment.EditedMay 8 2019, 7:15 AM


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

This revision was automatically updated to reflect the committed changes.