He's out for a few more weeks. It's on my radar, but I've been busy with other things.

Switch to using sizeof() inplace of prescribing VM_MAX_SUFFIXLEN

Fix and expand comments, add a compile time assert for safety.
Pad the buffer length in the sysctl handlers to the null terminator.

Just tested with TruOS/Trident from a May 2019 kernel, and it's getting ECAPMODE. Maybe the problems with Capsicum aren't worked out yet. I'll look some more at it.

Overall this looks very good, but error handoff in pvscsi_process_completion() is not correct. Generally if you're going to return a status that isn't CAM_REQ_CMP, you need to do the following before calling xpt_done():

I'm a bit lost on where the thread on sysctl_wire_old_buffer() is going. I'm fine with the code as-is, recommend not removing the call unless Mark advises otherwise.

Eh, I'm not a big fan of this, converting the driver to use strings from sysctl and kenv was intended to make it as easy as possible to set and unset flags from the default. It also adds new touch points in the system that will have to be updated or risk going stale if debugging evolves. Is editing, compiling, and deploying a new kernel easier than editing loader.conf? That said, I don't have a hard objection to it being committed, so long as the first time it goes stale, Warner buys me a nice chopped brisket dinner at Shorty's.

Add mpr too

Can you do the same for mpr?

Also, can you add the same changes for mpr?

One small thing to consider is whether to create a new typedef for cm_timeout_handler, even if it's the same implementation is mps_command_callback_t for now. The reason is that we might want to evolve this into a different function call signature for the timeout handler. This might be useful for mps_scsiio_timeout().

Switch to using an atomic swap for grabbing and clearing the reply
descriptor. This also undoes the unneeded churn from the last
revision. It makes the driver reliant on there being 64bit atomics,
which means that it won't work on powerpc anymore. C'est la vie!

I'm marking this as accepted despite my individual comments.

But CAM right now isn't great about knowing we're in 'shutdown' mode where error recovery might not be a good idea (but it might... It's unclear to me that we can say never here). In the mean time, doing the polling I think is a good 'make it better w/o letting the perfect be the enemy of the better' compromise.

I think that this is going in the right direction. The SSU/shutdown code in mpr/mps is still not great, IMHO. There's a completion handler for the SSU CCB that could take care of decrementing the SSU_refcount, but instead every I/O is checked for the SSU state in the normal command completion path, and the refcount decrement happens there. The cost of checking every I/O for the uncommon shutdown case is probably in the noise right now, but if we ever want these drivers to do millions of IOPS, it'll need to be addressed. Also, error recovery is going to be highly problematic. Protocol-level recovery is non-existent because the completion handler doesn't call cam_periph_error(), but transport-level error recovery (for things like timeouts) will likely still be called from the driver. Do we want error recovery to be triggered during a system shutdown, possibly triggering extra delays? If an error does happen, does ignoring it make the device problematic the next time the system starts up? If we want the driver to ignore transport level error recovery, how should that be flagged?

Unfortunately, there are some big drawbacks to this approach. The larger you make the chain frame pool, the more pressure you put on the contigmalloc allocator. This is usually (though not always!) not a problem at boot, but would be a big problem with loading the driver as a module after boot. I agree with the desire to make the chain frame pool be self-tuning, but I think that to do it the right way, the pool must be allocated in multiple smaller segments of no more than 2MB each (i.e. 16384 frames). It should also handle potential allocation failures. Without this, I feel that your approach will be too fragile.

In D14063#298088, @mav wrote:

I neither have major objections aside of one inline, nor too happy. From one side I agree that there is no any/much sense to return cam_status from functions like that, which do not work with command statuses. From the other side it is a) an API change, which may lead to difficult to find errors, but b) there are many other functions suffering the same problem (but which may be more invasive to fix though).

Good find on this. I've seen queue-full conditions in the past but hadn't tracked down the causes; I think this is it.

I think you forgot to add the implementation for CAM_PERIPH_PRINT(). Mine looks like this in cam_periph.h:

Something in this patch is causing my system to spontaneously reboot late in the boot process. No panic, and the messages on the screen disappear too quickly (and are erased in the serial console by the BIOS splash screen).

Looks good to me. I'm OK with removing scsi_low and all related code for FreeBSD 12.

I came across it during some other clean-up work.