The MsgLength field is in terms of DWORDs, i.e. 32bit words. A value of 16 means 64 bytes.

Ok, if the 2.5 msgver attribute is requiring 64bytes in some cases but 68bytes in other cases, that sucks, and it's a blatant violation of the 2.5 spec. Really, the motivation here was that any time a new msgver came out, investigation and review would be required to verify the sizes of the messages, instead of just blindly assuming sizes, but I forgot to put an assert into the CLI for that. I think that this would also impact the driver. I wonder if it’s possible to ask for just the first DWORD of the message and see what the firmware returns back as the residual, then ask for the rest based on that return value.

Please don't commit this. The whole point of the sysctl is to find out if you need to adjust the size of the buffer, and to provide an easy way to update the code as new versions of the MPI spec came out over time. At the time this code was written, 2.6 was the highest version of the spec, so that's all that was implemented here. But what if 2.7 came out and required +8? Or -4? Or some other variant? The intention was to leave open the door for other adjustments that might be required for versions later than 2.6, and my error was not providing a comment in the code, though this was hinted at in the commit message history. So the real problem is likely that the msgver is something numerically higher than 2.6, and the code needs to be updated to account for it. Please don't nerf it like you're trying to do in this patch.

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.