From 95fbded695ce52cdb6a2b2d8bab768bf7964d430 Mon Sep 17 00:00:00 2001 From: Scott Long Date: Fri, 7 Jun 2013 00:22:38 +0000 Subject: [PATCH] Simplify the checking of flags for cam_periph_mapmem(). This gets rid of a lot of code redundancy and grossness at very minor expense. Reviewed by: smh Obtained from: Netflix MFC after: 3 days --- sys/cam/cam_periph.c | 25 +++++++++++---------- sys/cam/scsi/scsi_pass.c | 45 ++++++++++---------------------------- sys/cam/scsi/scsi_sg.c | 33 +++++++++++++--------------- sys/cam/scsi/scsi_target.c | 17 ++------------ 4 files changed, 42 insertions(+), 78 deletions(-) diff --git a/sys/cam/cam_periph.c b/sys/cam/cam_periph.c index c3e3bd403761..f3300dbbacf5 100644 --- a/sys/cam/cam_periph.c +++ b/sys/cam/cam_periph.c @@ -681,9 +681,9 @@ camperiphfree(struct cam_periph *periph) /* * Map user virtual pointers into kernel virtual address space, so we can - * access the memory. This won't work on physical pointers, for now it's - * up to the caller to check for that. (XXX KDM -- should we do that here - * instead?) This also only works for up to MAXPHYS memory. Since we use + * access the memory. This is now a generic function that centralizes most + * of the sanity checks on the data flags, if any. + * This also only works for up to MAXPHYS memory. Since we use * buffers to map stuff in and out, we're limited to the buffer size. */ int @@ -728,9 +728,8 @@ cam_periph_mapmem(union ccb *ccb, struct cam_periph_map_info *mapinfo) case XPT_CONT_TARGET_IO: if ((ccb->ccb_h.flags & CAM_DIR_MASK) == CAM_DIR_NONE) return(0); - KASSERT((ccb->ccb_h.flags & CAM_DATA_MASK) == CAM_DATA_VADDR, - ("not VADDR for SCSI_IO %p %x\n", ccb, ccb->ccb_h.flags)); - + if ((ccb->ccb_h.flags & CAM_DATA_MASK) != CAM_DATA_VADDR) + return (EINVAL); data_ptrs[0] = &ccb->csio.data_ptr; lengths[0] = ccb->csio.dxfer_len; dirs[0] = ccb->ccb_h.flags & CAM_DIR_MASK; @@ -739,9 +738,8 @@ cam_periph_mapmem(union ccb *ccb, struct cam_periph_map_info *mapinfo) case XPT_ATA_IO: if ((ccb->ccb_h.flags & CAM_DIR_MASK) == CAM_DIR_NONE) return(0); - KASSERT((ccb->ccb_h.flags & CAM_DATA_MASK) == CAM_DATA_VADDR, - ("not VADDR for ATA_IO %p %x\n", ccb, ccb->ccb_h.flags)); - + if ((ccb->ccb_h.flags & CAM_DATA_MASK) != CAM_DATA_VADDR) + return (EINVAL); data_ptrs[0] = &ccb->ataio.data_ptr; lengths[0] = ccb->ataio.dxfer_len; dirs[0] = ccb->ccb_h.flags & CAM_DIR_MASK; @@ -812,8 +810,12 @@ cam_periph_mapmem(union ccb *ccb, struct cam_periph_map_info *mapinfo) } - /* this keeps the current process from getting swapped */ /* + * This keeps the the kernel stack of current thread from getting + * swapped. In low-memory situations where the kernel stack might + * otherwise get swapped out, this holds it and allows the thread + * to make progress and release the kernel mapped pages sooner. + * * XXX KDM should I use P_NOSWAP instead? */ PHOLD(curproc); @@ -885,8 +887,7 @@ cam_periph_unmapmem(union ccb *ccb, struct cam_periph_map_info *mapinfo) u_int8_t **data_ptrs[CAM_PERIPH_MAXMAPS]; if (mapinfo->num_bufs_used <= 0) { - /* allow ourselves to be swapped once again */ - PRELE(curproc); + /* nothing to free and the process wasn't held. */ return; } diff --git a/sys/cam/scsi/scsi_pass.c b/sys/cam/scsi/scsi_pass.c index eb5a57d03036..2b8b176017de 100644 --- a/sys/cam/scsi/scsi_pass.c +++ b/sys/cam/scsi/scsi_pass.c @@ -668,12 +668,11 @@ passsendccb(struct cam_periph *periph, union ccb *ccb, union ccb *inccb) { struct pass_softc *softc; struct cam_periph_map_info mapinfo; - int error, need_unmap; + xpt_opcode fc; + int error; softc = (struct pass_softc *)periph->softc; - need_unmap = 0; - /* * There are some fields in the CCB header that need to be * preserved, the rest we get from the user. @@ -687,28 +686,13 @@ passsendccb(struct cam_periph *periph, union ccb *ccb, union ccb *inccb) ccb->ccb_h.cbfcnp = passdone; /* - * We only attempt to map the user memory into kernel space - * if they haven't passed in a physical memory pointer, - * and if there is actually an I/O operation to perform. - * cam_periph_mapmem() supports SCSI, ATA, SMP, ADVINFO and device - * match CCBs. For the SCSI, ATA and ADVINFO CCBs, we only pass the - * CCB in if there's actually data to map. cam_periph_mapmem() will - * do the right thing, even if there isn't data to map, but since CCBs - * without data are a reasonably common occurrence (e.g. test unit - * ready), it will save a few cycles if we check for it here. - * - * XXX What happens if a sg list is supplied? We don't filter that - * out. + * Let cam_periph_mapmem do a sanity check on the data pointer format. + * Even if no data transfer is needed, it's a cheap check and it + * simplifies the code. */ - if (((ccb->ccb_h.flags & CAM_DATA_MASK) == CAM_DATA_VADDR) - && (((ccb->ccb_h.func_code == XPT_SCSI_IO || - ccb->ccb_h.func_code == XPT_ATA_IO) - && ((ccb->ccb_h.flags & CAM_DIR_MASK) != CAM_DIR_NONE)) - || (ccb->ccb_h.func_code == XPT_DEV_MATCH) - || (ccb->ccb_h.func_code == XPT_SMP_IO) - || ((ccb->ccb_h.func_code == XPT_DEV_ADVINFO) - && (ccb->cdai.bufsiz > 0)))) { - + fc = ccb->ccb_h.func_code; + if ((fc == XPT_SCSI_IO) || (fc == XPT_ATA_IO) || (fc == XPT_SMP_IO) + || (fc == XPT_DEV_MATCH) || (fc == XPT_DEV_ADVINFO)) { bzero(&mapinfo, sizeof(mapinfo)); /* @@ -726,13 +710,9 @@ passsendccb(struct cam_periph *periph, union ccb *ccb, union ccb *inccb) */ if (error) return(error); - - /* - * We successfully mapped the memory in, so we need to - * unmap it when the transaction is done. - */ - need_unmap = 1; - } + } else + /* Ensure that the unmap call later on is a no-op. */ + mapinfo.num_bufs_used = 0; /* * If the user wants us to perform any error recovery, then honor @@ -744,8 +724,7 @@ passsendccb(struct cam_periph *periph, union ccb *ccb, union ccb *inccb) SF_RETRY_UA : SF_NO_RECOVERY) | SF_NO_PRINT, softc->device_stats); - if (need_unmap != 0) - cam_periph_unmapmem(ccb, &mapinfo); + cam_periph_unmapmem(ccb, &mapinfo); ccb->ccb_h.cbfcnp = NULL; ccb->ccb_h.periph_priv = inccb->ccb_h.periph_priv; diff --git a/sys/cam/scsi/scsi_sg.c b/sys/cam/scsi/scsi_sg.c index 9836e2977775..1ad78ade98f0 100644 --- a/sys/cam/scsi/scsi_sg.c +++ b/sys/cam/scsi/scsi_sg.c @@ -946,25 +946,23 @@ sgsendccb(struct cam_periph *periph, union ccb *ccb) { struct sg_softc *softc; struct cam_periph_map_info mapinfo; - int error, need_unmap = 0; + int error; softc = periph->softc; - if (((ccb->ccb_h.flags & CAM_DIR_MASK) != CAM_DIR_NONE) - && (ccb->csio.data_ptr != NULL)) { - bzero(&mapinfo, sizeof(mapinfo)); + bzero(&mapinfo, sizeof(mapinfo)); - /* - * cam_periph_mapmem calls into proc and vm functions that can - * sleep as well as trigger I/O, so we can't hold the lock. - * Dropping it here is reasonably safe. - */ - cam_periph_unlock(periph); - error = cam_periph_mapmem(ccb, &mapinfo); - cam_periph_lock(periph); - if (error) - return (error); - need_unmap = 1; - } + /* + * cam_periph_mapmem calls into proc and vm functions that can + * sleep as well as trigger I/O, so we can't hold the lock. + * Dropping it here is reasonably safe. + * The only CCB opcode that is possible here is XPT_SCSI_IO, no + * need for additional checks. + */ + cam_periph_unlock(periph); + error = cam_periph_mapmem(ccb, &mapinfo); + cam_periph_lock(periph); + if (error) + return (error); error = cam_periph_runccb(ccb, sgerror, @@ -972,8 +970,7 @@ sgsendccb(struct cam_periph *periph, union ccb *ccb) SF_RETRY_UA, softc->device_stats); - if (need_unmap) - cam_periph_unmapmem(ccb, &mapinfo); + cam_periph_unmapmem(ccb, &mapinfo); return (error); } diff --git a/sys/cam/scsi/scsi_target.c b/sys/cam/scsi/scsi_target.c index 56ff8a908dc9..78e96fb6b767 100644 --- a/sys/cam/scsi/scsi_target.c +++ b/sys/cam/scsi/scsi_target.c @@ -726,21 +726,8 @@ targsendccb(struct targ_softc *softc, union ccb *ccb, ccb_h->cbfcnp = targdone; ccb_h->targ_descr = descr; - /* - * We only attempt to map the user memory into kernel space - * if they haven't passed in a physical memory pointer, - * and if there is actually an I/O operation to perform. - * Right now cam_periph_mapmem() only supports SCSI and device - * match CCBs. For the SCSI CCBs, we only pass the CCB in if - * there's actually data to map. cam_periph_mapmem() will do the - * right thing, even if there isn't data to map, but since CCBs - * without data are a reasonably common occurrence (e.g. test unit - * ready), it will save a few cycles if we check for it here. - */ - if (((ccb_h->flags & CAM_DATA_MASK) == CAM_DATA_VADDR) - && (((ccb_h->func_code == XPT_CONT_TARGET_IO) - && ((ccb_h->flags & CAM_DIR_MASK) != CAM_DIR_NONE)) - || (ccb_h->func_code == XPT_DEV_MATCH))) { + if ((ccb_h->func_code == XPT_CONT_TARGET_IO) || + (ccb_h->func_code == XPT_DEV_MATCH)) { error = cam_periph_mapmem(ccb, mapinfo);