Make sure we check for CAM_CDB_POINTER for all drivers. Also, for the drivers I've touched, filter out CAM_CDB_PHYS.
ClosedPublic
Actions

Authored by imp on Mar 8 2016, 10:07 PM.

Details

Reviewers

scottl
ken

Commits

rS312850: MFC r296891 (by imp):
rS296891: Make sure we check for CAM_CDB_POINTER for all drivers. Also, for the

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2826
Build 2850: arc lint + arc unit

Event Timeline

imp updated this revision to Diff 14176.Mar 8 2016, 10:07 PM

imp retitled this revision from to Make sure we check for CAM_CDB_POINTER for all drivers. Also, for the drivers I've touched, filter out CAM_CDB_PHYS..

imp updated this object.

imp edited the test plan for this revision. (Show Details)

imp added reviewers: scottl, ken.Mar 8 2016, 10:08 PM

scottl added inline comments.Mar 8 2016, 10:16 PM

sys/dev/isci/isci_controller.c
748	What do you think about this stylistic change: if (ccb->ccb_h.flags & (CAM_CDB_POINTER \| CAM_CDB_PHYS)) {
sys/dev/ppbus/vpo.c
195	Does this block need to get moved up here? Was it moved accidentally?

imp added inline comments.Mar 8 2016, 10:23 PM

sys/dev/isci/isci_controller.c
748	that's subtly different. However, in other places in the tree we just check CAM_CDB_PHYS, and I could do that here as well.
sys/dev/ncr/ncr.c
3866–3867	I could just check CAM_CDB_PHYS here, as is done in other places in the tree.
sys/dev/ppbus/vpo.c
195	No. It was done on purpose so we could compute ptr just once. And it doesn't change anything in terms of what is printed since we always call vpo_intr from the place it was moved from, and only from there.

scottl added inline comments.Mar 8 2016, 10:32 PM

sys/dev/isci/isci_controller.c
748	I suppose that the driver can accidentally handle CAM_CCB_POINTER thanks to the magic of relaxed syntax of referencing pointers vs arrays, and it never tries to reference a single byte in the array directly. CAM_CCB_PHYS has to imply CAM_CCB_POINTER. You could just remove the check for CAM_CCB_POINTER.
sys/dev/ppbus/vpo.c
195	I don't understand. The existing design style was to print the various commands in vpo_action(). What you've changes is not technically incorrect, but it's now inconsistent.