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
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I've added some comments inline.
| sys/dev/mpr/mpr.c | ||
|---|---|---|
| 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. |
| sys/dev/mpr/mpr_sas.c | ||
| 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. |
| sys/dev/mpr/mpr.c | ||
|---|---|---|
| 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. |
| sys/dev/mpr/mpr_sas.c | ||
| 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. |
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 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? |
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 1936 ↗ | (On Diff #26542) | FW guy says NVMe commands are typically executed in parallel. |
| sys/dev/mpr/mpr.c | ||
|---|---|---|
| 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 |
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 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. |
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 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. |
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 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. |
Comment Actions
I have no objections aside of ones you already promised to fix.
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 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? |
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 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. |
Comment Actions
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:
mpr.c:
Line 1404 - add comment Line 1408 - use sc->maxio
mpr_sas.c:
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
mprvar.h:
Line 39 - remove MPR_MAX_IO_SIZE Line 290 - define maxio