Page MenuHomeFreeBSD

Add tri-mode support to mpr

Authored by slm on Mar 22 2017, 3:53 PM.



Add tri-mode support (SAS/SATA/PCIe), also known as Ventura Gen3.5 support, to the mpr driver. This includes support for NVMe devices.

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

slm created this revision.Mar 22 2017, 3:53 PM
mav edited edge metadata.Mar 22 2017, 6:02 PM

I've added some comments inline.

1405 ↗(On Diff #26542)

Is this 1MB limit is based on something or at least reported outside? Now this driver reports maximal IO size via cpi->maxio in XPT_PATH_INQ, and it looks much more based on some real limitations. FreeBSD limits IOs to MAXPHYS constant, which is 128KB by default, but it can be overridden in kernel config.

1936 ↗(On Diff #26542)

Is there something wrong with firmware doing it? It looks dirty to me when parts of command set are handled in different places.

2023 ↗(On Diff #26542)

It just should not happen if initiator periph driver is sane.

2024 ↗(On Diff #26542)

I think any way IOCDBLEN is more appropriate here then 16.

2026 ↗(On Diff #26542)

I've never seen such error code to be used before, and I don't see it handled anyhow special. CAM now handles CAM_REQ_INVALID better when some CDB is not supported.

2913 ↗(On Diff #26542)

Instead of csio.cdb_io.cdb_bytes there is scsiio_cdb_ptr() macro now to handle CAM_CDB_POINTER case.

slm added inline comments.Mar 22 2017, 7:08 PM
1405 ↗(On Diff #26542)

I did this change before I added the cpi->maxio change and I forgot about it. I'll change this to use cpi->maxio instead.

1936 ↗(On Diff #26542)

I did not add the UNMAP change and I'm not sure of the background. I'll check with the person who did it to answer your questions in this area.

2023 ↗(On Diff #26542)

It probably makes sense to use a KASSERT here, then we can remove this code all together.

2913 ↗(On Diff #26542)

OK. We'll probably need to check the FreeBSD version number then, if we MFC this code. And I think there are other places that should change as well.

ken accepted this revision.Mar 22 2017, 8:07 PM

In general this looks good. I agree with mav's comments.

This revision is now accepted and ready to land.Mar 22 2017, 8:07 PM
ken added inline comments.Mar 22 2017, 8:09 PM
1936 ↗(On Diff #26542)

Another question about UNMAP -- do the NVME passthrough commands get single-stepped through the firmware like ATA passthrough commands or are they executed in parallel with other outstanding NVME commands?

slm added inline comments.Mar 22 2017, 8:56 PM
1936 ↗(On Diff #26542)

FW guy says NVMe commands are typically executed in parallel.

scottl added inline comments.Mar 22 2017, 9:13 PM
1005 ↗(On Diff #26542)

Something to be done in the future would be to have separate atomic/non-atomic handlers, and eliminate references to the softc. Not a big deal now, but will be important with multi-queue to get to 1Mpps

scottl added inline comments.Mar 22 2017, 9:18 PM
2023 ↗(On Diff #26542)

It looks like CDB length of 32 is required for EEDP, and I think that this change is going to break that. I'm a little confused why this chunk of code needed to change as part of Tri-mode support.

slm added inline comments.Mar 22 2017, 9:38 PM
2023 ↗(On Diff #26542)

This part has nothing to do with Tri-Mode. There was a merge conflict with the Current driver and our driver. We fixed this part in one way and Alan fixed it another I think is what happened. We can change it back to the KASSERT and it should be OK.

slm added inline comments.Mar 23 2017, 4:17 PM
1936 ↗(On Diff #26542)

This is the answer to why the driver issues UNMAP commands instead of FW:

This translation is necessary because in some I/O workloads SCSI Unmap commands can comprise up to 20% of the I/O commands. Issuing these using the MPI SCSI IO request will require F/W to translate the command into an NVMe native command. To maximize performance, OS drivers should translate SCSI Unmap commands to NVMe Encapsulated commands to be handled directly by H/W acceleration.

mav accepted this revision.Mar 23 2017, 4:23 PM

I have no objections aside of ones you already promised to fix.

1936 ↗(On Diff #26542)

Thanks for the answer. Though I can't say that I like it. It creates another rhetorical question: why they should we bother to make firmware to translate other commands and translate this command ourselves?

slm added inline comments.Mar 23 2017, 4:42 PM
1936 ↗(On Diff #26542)

I don't know for sure, but I assume that we did some testing and determined that, in some cases, if the driver handles the UNMAP command it frees up FW to handle other I/Os. My guess is that this is just a special case where we can predict that a little help from the driver will improve performance, whereas it wouldn't make sense in most other cases. If the driver handled all commands, I think that would hurt performance quite a bit.

slm updated this revision to Diff 26899.Mar 31 2017, 10:32 PM
slm edited edge metadata.

I'm not sure there is a better way to update my changes without checking them in first, which I didn't do. So, the changes I made from the original review are:


Line 1404 - add comment
Line 1408 - use sc->maxio


Line 1031 - set sc->maxio
Line 1716 - use scsiio_cdb_ptr macro or check for CAM_CDB_POINTER
Line 1856 - add scsi_opcode declaration
Line 1948 - use scsiio_cdb_ptr macro or check for CAM_CDB_POINTER
Line 2040 - revert to using KASSERT
Line 2562 - add scsi_cdb pointer declaration
Line 2661 - use scsiio_cdb_ptr macro or check for CAM_CDB_POINTER
Line 2681, 2723, 2818, 2832, 2847, 2939 - use scsi_cdb
Line 2727 - use csio


Line 39 - remove MPR_MAX_IO_SIZE
Line 290 - define maxio
This revision now requires review to proceed.Mar 31 2017, 10:32 PM
ken accepted this revision.May 16 2017, 8:50 PM

Looks good to me, thanks!

This revision is now accepted and ready to land.May 16 2017, 8:50 PM
scottl accepted this revision.May 16 2017, 9:23 PM
This revision was automatically updated to reflect the committed changes.