Change fwload_only to fwload_force.

In D45660#1042762, @mav wrote:

In D45660#1042742, @ken wrote:

So here is what the debugging log message in isp_getpdb() shows. isp0 and isp1 are connected to LTO-6 tape drives via an 8Gb switch. isp2 is directly connected to an LTO-6 in loop mode:

isp0: Chan 0 handle 0x0 Port 0xfffc01 flags 0x0 curstate 77 laststate 77
isp0: Chan 0 handle 0x1 Port 0x011b26 flags 0x40a0 curstate 46 laststate 46
isp0: Chan 0 handle 0x7fe Port 0xfffffe flags 0x0 curstate 44 laststate 44
isp0: Chan 0 handle 0x7fe Port 0xfffffe flags 0x0 curstate 44 laststate 44
isp1: Chan 0 handle 0x0 Port 0xfffc01 flags 0x0 curstate 77 laststate 77
isp1: Chan 0 handle 0x1 Port 0x011a26 flags 0x40a0 curstate 46 laststate 46
isp1: Chan 0 handle 0x7fe Port 0xfffffe flags 0x0 curstate 44 laststate 44
isp1: Chan 0 handle 0x7fe Port 0xfffffe flags 0x0 curstate 44 laststate 44
isp2: Chan 0 handle 0x0 Port 0x000026 flags 0x40a0 curstate 46 laststate 46

So it seems the upper 4 bits can actually have different values, not some "non-NVMe" constant, as I thought. I am OK with this patch if it solves the issue, but it would definitely be good to understand what it is, in case there is something important.

In D45688#1042676, @mav wrote:

It seems a good tunable, except I am not getting the meaning of "only" there. Why not "always", "force" or something like that?

In D45660#1042675, @mav wrote:

None of QLogic documents I have know nothing about NVMe, and this state field is declared is byte there. I have no objections for this patch, but a bit curios what NVMe status do we see there for non-NVMe devices.

Looks like the copyright text is missing.

Looks good, thank you for doing this! I'm glad to finally see CTL NVMe support going in.

This should be fine, thanks!

Looks fine to me, thanks!

Use BUS_SPACE_MAXSIZE_32BIT+1 to represent the 4GB boundary instead of 0x100000000.

Good idea. Sbuf didn't exist back when the debug macros were written, this is an improvement.

Looks good, thank you!

This makes it sound like unmapped I/O and rotating media support will be removed in FreeBSD 15.

Looks ok to me.

Looks good to me.

Looks fine to me.

In D41167#937442, @mav wrote:

My position traditionally was that user-space should handle and report its own errors by itself, but kernel should handle some system-wide. I agree that there may be some merits in this. Aside of reporting to devctl this should also properly re-broadcast Unit Attentions, which do not belong to specific command and so periph. As I can see, devctl should already receive device name to differentiate pass-through requests. I wonder it is could report some process identification too?

I think this will be ok. It will give at least some avenue to report errors that go up via the pass(4) driver without additional console spamming.

Looks fine, thanks.

Looks good to me, thanks.

Looks ok to me.

This looks good to me.

Create a cam_strvis_flag() function and use it in the NVMe code.

I'm assuming this will need to run on a system with a SES device.

Hmm, I guess that was removed somewhere along the way. I haven't seen discovery hangs lately, so this should be ok.

Update sa(4) comments and man page after review.

Address documentation feedback, clarify that loader tunables will still be
used for tape drives that arrive after boot.

In D33783#764114, @imp wrote:

In D33783#764113, @rew wrote:

Does it make sense to include any of these additional errors for the CAM_IO_STATS in sys/cam/nvme/nvme_da.c and sys/cam/ata/ata_da.c?

nvme_da doesn't have these errors. ata_da already lists CAM_ATA_STATUS_ERROR. I needed to add it to SCSI because we do a lot with ATA passthrough commands that might result in this. I'm on the fence about adding CAM_SMP_STATUS_ERROR to ata_da.c, like I was in adding it here. It's more of a management thing and I've not audited all the commands both send to see if it's possible... CAM_SEL_TIMEOUT is generated by the ata SIMs, so maybe it should be on the list there (nvme_sim doesn't generate this).

In D33783#764106, @asomers wrote:

What's the difference between CAM_CMD_TIMEOUT and CAM_SEL_TIMEOUT ?

In D29941#676918, @asomers wrote:

Yep. That's wrong. In 14.0-CURRENT, the function that sets physpath is ses_set_physpath, in cam/scsi/scsi_enc_ses.c.

In D29941#676906, @asomers wrote:

In D29941#676901, @mav wrote:

Excuse me if I am wrong, I don't have any multipath environment now to make sure, but I remember that SES on different expanders of dual-port backplane report different IDs.

Hm. I thought that CAM used the Enclosure Logical Identifier to generate the physpath. It *should* do that. But now that I double-check; you're right. CAM is using the Addressed Logical Unit SAS ID instead. Maybe it was a Spectra-specific patch to use the ELI that never got upstreamed. @ken could you please check how SpectraBSD generates the physical path? You can use commands like

sudo diskinfo -p /dev/da0
sudo sg_ses -p1 /dev/ses0 | grep -i 'enclosure.*logical'
sudo sg_inq -p di /dev/ses0

One minor comment.

The CDBs and filling functions look good. We should figure out what our user interface idiom is going to be for the scsi_wrap_* functions. Do we assume we have stdout/stderr, or do we provide those strings for the user to print in a way that works for the calling application?