Page MenuHomeFreeBSD

cam: enable kern.cam.ada.enable_uma_ccbs by default
ClosedPublic

Authored by trasz on May 31 2021, 12:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 30, 2:35 PM
Unknown Object (File)
Wed, Oct 30, 4:29 AM
Unknown Object (File)
Oct 3 2024, 7:25 AM
Unknown Object (File)
Oct 2 2024, 9:19 PM
Unknown Object (File)
Sep 30 2024, 11:30 PM
Unknown Object (File)
Sep 26 2024, 6:07 PM
Unknown Object (File)
Sep 25 2024, 10:53 PM
Unknown Object (File)
Sep 21 2024, 5:14 AM
Subscribers

Details

Summary

This makes the ada(4) driver use UMA for its CCBs.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

We've had good luck with this one. I'd feel better if we could have a quick coccinelle script to find this... Ignore the "this is bogus" because they might not be...

// Script to find all the uses of ccb's allocated on the stack and change
// them to use the new setup function which bzeros before doing a normal
// setup of the CCB.
@@
union ccb *CCB1;
union ccb *CCB2;
expression E;
@@
- memcpy(CCB1,CCB2,E)
+ THIS_IS_BOGUS(CCB1,CCB2,E)

@@
union ccb *CCB1;
union ccb *CCB2;
expression E;
@@
- bcopy(CCB1,CCB2,E)
+ THIS_IS_BOGUS(CCB1,CCB2,E)

@@
union ccb *CCB1;
union ccb *CCB2;
@@
- *CCB1=*CCB2
+ THIS_IS_BOGUS(CCB1,CCB2)

Had the following hits:

diff -u -p a/cam_xpt.c b/cam_xpt.c
--- a/cam_xpt.c
+++ b/cam_xpt.c
@@ -487,7 +487,7 @@ xptdoioctl(struct cdev *dev, u_long cmd, caddr_t addr,
 			xpt_path_lock(ccb->ccb_h.path);
 			cam_periph_runccb(ccb, NULL, 0, 0, NULL);
 			xpt_path_unlock(ccb->ccb_h.path);
-			bcopy(ccb, inccb, sizeof(union ccb));
+			THIS_IS_BOGUS(ccb, inccb, sizeof(union ccb));
 			xpt_free_path(ccb->ccb_h.path);
 			xpt_free_ccb(ccb);
 			break;
@@ -518,7 +518,7 @@ xptdoioctl(struct cdev *dev, u_long cmd, caddr_t addr,
 				      inccb->ccb_h.pinfo.priority);
 			xpt_merge_ccb(&ccb, inccb);
 			xpt_action(&ccb);
-			bcopy(&ccb, inccb, sizeof(union ccb));
+			THIS_IS_BOGUS(&ccb, inccb, sizeof(union ccb));
 			xpt_free_path(ccb.ccb_h.path);
 			break;
 		}
diff -u -p a/scsi/scsi_pass.c b/scsi/scsi_pass.c
--- a/scsi/scsi_pass.c
+++ b/scsi/scsi_pass.c
@@ -2232,7 +2232,7 @@ passsendccb(struct cam_periph *periph, union ccb *ccb,

 	ccb->ccb_h.cbfcnp = NULL;
 	ccb->ccb_h.periph_priv = inccb->ccb_h.periph_priv;
-	bcopy(ccb, inccb, sizeof(union ccb));
+	THIS_IS_BOGUS(ccb, inccb, sizeof(union ccb));

 	return(0);
 }
diff -u -p a/cam_periph.c b/cam_periph.c
--- a/cam_periph.c
+++ b/cam_periph.c
@@ -1423,7 +1423,7 @@ camperiphdone(struct cam_periph *periph, union ccb *do
 	 * and the result will be the final one returned to the CCB owher.
 	 */
 	saved_ccb = (union ccb *)done_ccb->ccb_h.saved_ccb_ptr;
-	bcopy(saved_ccb, done_ccb, sizeof(*done_ccb));
+	THIS_IS_BOGUS(saved_ccb, done_ccb, sizeof(*done_ccb));
 	xpt_free_ccb(saved_ccb);
 	if (done_ccb->ccb_h.cbfcnp != camperiphdone)
 		periph->flags &= ~CAM_PERIPH_RECOVERY_INPROG;
@@ -1713,7 +1713,7 @@ camperiphscsisenseerror(union ccb *ccb, union ccb **or
 			 * this freeze will be dropped as part of ERESTART.
 			 */
 			ccb->ccb_h.status &= ~CAM_DEV_QFRZN;
-			bcopy(ccb, orig_ccb, sizeof(*orig_ccb));
+			THIS_IS_BOGUS(ccb, orig_ccb, sizeof(*orig_ccb));
 		}

 		switch (err_action & SS_MASK) {

There's a review for the cam_periph one. The two ioctls in cam_xpt.c I think are safe because we're copying like to like in terms of the CCB allocation. Ditto with the scsi_pass one.

Based on this, I think we're good, but wanted to "show my work" in case there's some flaw in it.

This revision is now accepted and ready to land.Jul 6 2021, 3:25 PM

The ioctl ones may be a copyout in disguise too, but if so, that's also safe.

In D30567#699123, @imp wrote:

The ioctl ones may be a copyout in disguise too, but if so, that's also safe.

Now that I've had time to look at the code in full context, the ioctl functions are definitely a copyout, so they are fine for sure. And the path that did trigger is only for da, not ada. So I'm quite convinced now.