A minor follow-up to this would be to use macros for the serial and model field lengths, and update ata.h to use the same, so that there's no reliance on sizeof.

I know it's been almost 2 months, sorry for the delay. Luiz is about to go on vacation for a few days, hopefully he'll review this once he gets back

I'm not sure what you're asking for at first that's not included in the summary

I've put rate limiters into other drivers and have been unhappy with the results. Whether it's once a second, 10 seconds, 100 seconds, or whatever, an unattended system will wind up with thousands of messages in its logs that obfuscate more important messages. I think a better tactic is to print a message once, and maintain a sysctl with a counter.

In D30298#701262, @trasz wrote:

So... Scott, are you still against the change as it is?

This is the kind of change that seems simple and obvious, but opens up semantic gray areas in the language and the runtime. Is the intent that 0 means that trim is disabled? Should this coordinate with other tunable? How would one express unlimited? I guess as 0x7ffffffffffffff since it's a signed quad? It's now possible to feed a negative value into d_delmaxsize where that wasn't possible before (even though the field is signed), so does the rest of the system know how to deal with that?

The scsi_ctl.c part needs a lot of work. ctlferegster() is probably allocating the ccb on the stack because it's the peripheral constructor and is being called with locks held, so the original author didn't want to do a malloc(M_WAITOK). This is a pattern in CAM that needs to be fixed but is outside of the scope of this change. I think that that's probably a better to replace the stack allocations with malloc(M_NOWAIT|M_ZERO) and not overthink an out-of-memory situation. But if that's not acceptable, then consider that the ccb is only used for XPT_FE_LUN and doesn't need a full union ccb allocation on the stack. I'm also a bit confused and concerned as to why this function uses different allocation paradigms; pick one and stick with it.

Thank you. This stuff always broke my brain.

The model is that a sim represents the queueing and resource pool for one or more buses. If multiple buses have their own private resource and scheduling pools, then those buses need to have separate sims. If they share resources, then they need to share the same sim. AHC was weird because even though it had a split "TWIN" mode, apparently the split buses still had independent resource pools and queueing limits. The same was not true with cards that presented logical and physical drives like mpt and ciss; they still draw from the same resource pool, so they needed a single SIM and also need distinctive buses so the physical and logical target numbers don't conflict. This is really a detail of the driver and hardware, and is something that the driver writer should know and understand early in the design phase of development. Even though SAS controllers have multiple "ports", the queueing is still typically common to all ports on the device, so only a single bus is created on the sim.

Yeah, my feeling is still that C function return values should be errno's, and that cam_status should be reserved for use primarily in the status fields of CCBs and related structures. If a C function needs to return a cam_status, it should return an errno and pass the cam_status by reference.

Part of me feels that abstracted definitions for implementation semantics is good API design, but it sounds like this is a good candidate for garbage collection.

This looks like a deficiency in the API design, but I don't object to this change.

In D28821#647027, @markj wrote:

In D28821#647026, @scottl wrote:

I won't be able to go back and test this for at least a few more days. However, when I tested the atomic_load_int() suggestion over the weekend, I got a really weird panic instead. I didn't dig into it because of other time pressures, but it does make me worry about compiler bugs and other weirdness, whereas the membar is proven to work for now.

Looking at the patch you pasted at the time, you had if (atomic_load_int(tracker->rmp_flags)) instead of if (atomic_load_int(&tracker->rmp_flags)). I'm not sure how the former would compile (though it is more plausible on armv7), but if it did I'd indeed expect a panic.

I won't be able to go back and test this for at least a few more days. However, when I tested the atomic_load_int() suggestion over the weekend, I got a really weird panic instead. I didn't dig into it because of other time pressures, but it does make me worry about compiler bugs and other weirdness, whereas the membar is proven to work for now.