- User Since
- Jun 4 2014, 7:07 AM (334 w, 12 h)
Mon, Oct 26
Wed, Oct 21
Sun, Oct 18
There's nothing wrong with this work, but it doesn't solve the real problem that CCBs have grown too large, and that the one-size-fits-all union concept for them doesn't scale. The wasted 800 bytes is not even the biggest problem; a lot more is wasted within the union size of the CCB for regular I/O. This scheme needs to be replaced with variable-sized CCBs at a minimum, and the whole CCB concept needs to be analyzed and re-designed for high IOP workloads. So with that said, I won't block this review, but I urge people to think about these problems and don't stop with just this patch.
Tue, Oct 13
Sat, Oct 10
Fri, Oct 9
Sep 28 2020
Sep 25 2020
Sep 14 2020
Sep 11 2020
This change isn't bad, but I've wanted FreeBSD to get away from the 14 argument bus_dma_tag_create() and friends and move to the more compact bus_dma_template_tag(). Would it be possible to take this patch in that direction instead? Maybe add bus_dma_template_init_dev(&t, dev) to go alongside the existing bus_dma_template_init(&t, parent)
Yeah, my reading is that this code is not compliant with the spec, and that changing strncpy() to strlcpy() won't help. What I would probably do is something list this:
Sep 2 2020
Yeah, what Warner is thinking of is when sim->mtx == &Giant. I think we've removed all of that code.
Aug 7 2020
Jul 31 2020
Whatever reason there was for either using M_WAITOK or for checking for failure are lost to time.
Jul 9 2020
Jul 7 2020
Apr 22 2020
In a pinch, I'd take it back to being a page size. I can see an argument that oversized CDBs and CAM_CDB_POINTER are underused to the point of creating unnecessary complication and risk to the code (as is evident from Alexander breaking it in r307205). No API was ever developed to make the feature easier to use, so it's been hit-and-miss on whether SIMs even support it. Maybe we eliminate it and deal with oversized CDBs via a new CCB type. We should also look into whether the embedded layout is creating unnecessary cache pollution in the CCB; maybe it makes sense to always have the CDB be a pointer to a scratch area at the end of the CCB that can grow as needed. Either way, we could bring everything into a conforming pattern that's easier and less risky.
Apr 21 2020
Blah, but thanks for the history on it, I guess I never noticed before. I would be surprised if this hasn't caused bugs. There's no longer an active standards group, and even if there was I'd advocate that this is wrong. Let's follow up with a change on it.
The point of all of this is that CDB's were predicted to grow larger than 16 bytes, and CAM was trying to optimize the common case of 6/10/12/16 byte CDB's without creating an API incompatibility for handling future growth. If a device came along that needed 32 byte CDB's, you could still use the stock CCB fields, but tack on the CDB as a separate allocation rather than have it be embedded in the CCB. So, while I agree with the original problem statement that the use of alloca() was unsafe, I disagree with the resolution.
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.
Yep, can tackle the CAM_DIR problem separately.
Apr 19 2020
Apr 16 2020
Apr 15 2020
Works on my Icelake system
Mar 31 2020
Works for my Icelake laptop Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
Mar 28 2020
Looks really good.
Mar 20 2020
Mar 10 2020
Mar 9 2020
Mar 1 2020
Feb 28 2020
FC isn't part of the problem here, so I ask that we stop reverting to it in the discussion.
Feb 27 2020
I think it's much less likely that a SIM will internally map UINT_MAX to a valid target than what has already been demonstrated where it'll map an errant initiator_id to a valid target.
Initiator ID has no meaning for SAS initiators, your comment about this function scanning targets that it should not within the domain of XPORT_SAS doesn't make sense to me. Numerical bus ID's don't even make sense within SAS topology, and at some point we need to fix all of this to use port UUIDs as the definitive identifier. We also need to fix the entire SIM API and model so that the buses are truly self-announcing and self-identifying, not this crummy hybrid we have of still pretending that the SAS transport is sequentially scanned like parallel SCSI is. But until that happens, what Andriy is proposing is a seat belt for XPORT_SAS initiator SIMs so that they don't shoot themselves in the foot due to the poor API that we've provided.
The patch only impacts XPT_SCAN_TGT/XPT_SCAN_BUS operations that don't apply to target SIMs. Again, I see no reason not to proceed.
This commit only affects the behavior of XPT_SCAN_TGT/XPT_SCAN_BUS and has no bearing on the behavior of target SIMs. It also is specific to XPORT_SAS, and has no effect on XPORT_FC. I see no reason for it not to proceed.
This is good enough for now. Long term the transport needs to move to auto discovery instead of active scanning.
Feb 17 2020
Feb 16 2020
Feb 10 2020
Feb 7 2020
I agree with Alexander, I really don't like that this perpetuates bad design.
Jan 30 2020
Also, this should be MFC'd to 12 and even 11. With that, you can remove in 13 instead of waiting for 14
My recommendation is to change the rotating and unmapped sysctls to be SYSCTL_PROC, and have them read the flag. That way you can retire the fields out of the softc now.
Jan 15 2020
Jan 2 2020
Dec 26 2019
Dec 24 2019
Dec 23 2019
Change function names for better consistency. Add bus_dma_template_clone().
Update the man page.
I also have an large update to bus_dma.9 that I'll add to the review.
I already changed the names in an upcoming revision; I agree that simple wasn't a good name. For the purposes of cloning an existing tag, what I'd propose is to have a function, bus_dma_template_clone(*template, *dmat) that serializes the opaque fields of the tag back into a template, then lets you optionally modify the template, and then turn it into a new tag with bus_dma_template_tag(*template, *tag). I'll code that up and submit it in the next patch.
Switch to a typedef for the template. Be type-correct with
NULL field assignments.