PR209468: aacraid fixes to work with newer controller firmware
ClosedPublic
Actions

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

Details

Reviewers

scottl
scott.benesh_microsemi.com
mav
imp

Commits

rS348091: Make aacraid(4) working on ASR8805 & ASR8402 in particular. This patch

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 - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

yuripv created this revision.Dec 2 2018, 4:04 PM

Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptDec 2 2018, 4:04 PM

Now with full context...

sbruno added reviewers: scottl, scott.benesh_microsemi.com.Dec 2 2018, 4:14 PM

sbruno added a reviewer: mav.

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

IMO go ahead and commit

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

scott.benesh_microsemi.com added inline comments.Dec 3 2018, 5:32 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.Dec 3 2018, 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.

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?

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 .