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?

A couple of small nits but overall this looks good.

In D24489#539455, @scottl wrote:

The problem isn't the existence of bi-directional commands, it's that DIR_BOTH == 0, not an OR'ing of DIR_IN and DIR_OUT, as would be expected.

I think this is ok. Do we need to do anything for compatibility?

In D23852#524939, @mav wrote:

I am OK with that, but I think it can be too easy to forget, same as wrong value is set without understanding now. I am thinking whether introduction of some special constant value for initiator_id equal to UINT_MAX or something could be more explicit?

In D23852#524775, @mav wrote:

In D23852#524734, @avg wrote:

I would expect that if an initiator can also be a target then its target ID -- which, if I understand correctly, you suggest to be treated as an initiator ID -- would be from a different namespace than IDs of its targets.

Why? Think about it as parallel SCSI or loop FC, where each port can be both initiator and target, and have ID from some namespace. That is how initiator_id originally appeared in CAM, as I understand. We actually had some parallel SCSI target support in base, it just died with the hardware.

That is, I think that for a SAS initiator it is impossible to see itself as a target (where a target ID is typically also some made up number derived from the actual SAS topology).

I would not call it impossible. It is just not very useful. For QLogic FC IIRC it partially even works, but I prefer it not to rather then try and fail.

Also, about your other point, it's not hard to fake an initiator ID in every SAS SIM and all of them (have to) do it already.
But I do not see a reason to keep doing that when it does not have any real use except for introducing occasional bugs.

As I have told it works reasonably for FC, it was reasonable for SPI, and I see no reason why it can not for other protocols, that would justify protocol specifics into the shared code.

@ken , you've been around the target code for longer time, what do you think?

Good catch, thank you!

In D20282#437525, @chuck wrote:

In D20282#437333, @imp wrote:

This looks good. The multiple printfs give me heart-burn, but that's for another day. This part of the code they should be safe, but they aren't safe in a future where we do device discovery in parallel.

It would be easy to turn this into a series of snprintfs() and then a single printf(). Would that clear the heartburn?

Looks good to me.

I think it would actually be better to add the suggestion to put in ispfw_load whenever the user isn't on a 16Gb board and hasn't loaded firmware.

I tested this out with a Seagate 8TB host aware drive:

In D19249#411871, @asomers wrote:

This change looks good. However, zonectl(8) currently hard-codes the entries_allocated field and doesn't check for a short return. If the kernel truncates the zone list, the user will have no obvious way of knowing. I think zonectl(8) should be modified to either loop until all zones have been reported, or at least tell the user that not all zones were reported.

Looks good. I assume you've made sure this installs correctly. I put a couple of suggestions in the description file, although the comment about MAXPHYS might also go into something that the user will see when he installs the package.

Is the doc tree in freeze like the src tree? i.e. does he need permission from doceng?

No idea why @mat blocked it, but I'm sure he can explain.