Maybe I’m confused, I was referring to tb_pcib.c.

Either return the PCI IDs in the tb_pcib driver, or remove the entire driver. With this review, the entire driver is dead code.

I must decline being a reviewer or otherwise being involved in the freebsd code. Happy to discuss publicly available information regarding the tech in general.

What I meant by "kinda recommending" is that if you're going to support TB3 PCI IDs, then bring back the ICM code until you've written an HCM to replace it. Otherwise you're relying on people to find the right option in their BIOS to disable the ICM challenge-response authorization exchange with devices.

The AR_2C_NHI Ids probably came from https://admin.pci-ids.ucw.cz/read/PC/8086/1575. I never had pre-production hardware.

The relationship between TB3 and USB4 isn't as simple as you suggest. There are optional vs mandatory feature considerations, there are differences in the bus properties and device config registers, there are differences in the necessary behavior of the Connection Manager, not to mention that an HCM is mandatory for USB4 but not for TB3, and there are even differences in the NHI programming interface. A better comparison is TB4 and USB4, their programming interfaces and functional attributes are more closely related, with TB4 being a superset of USB4.

If the hardware falls into the MRSAS family then there's a better chance that it'll work with just a PCI ID addition to the driver. Doesn't hurt to try.

Did any see that flash of smoke or smell sulphur and brimstone just now? Oh, that's just me....

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.