From owner-svn-src-head@FreeBSD.ORG Fri Jun 7 00:22:40 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 60E94B24; Fri, 7 Jun 2013 00:22:40 +0000 (UTC) (envelope-from scottl@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:1900:2254:2068::e6a:0]) by mx1.freebsd.org (Postfix) with ESMTP id 51F261E0E; Fri, 7 Jun 2013 00:22:40 +0000 (UTC) Received: from svn.freebsd.org ([127.0.1.70]) by svn.freebsd.org (8.14.7/8.14.7) with ESMTP id r570MdQE065054; Fri, 7 Jun 2013 00:22:39 GMT (envelope-from scottl@svn.freebsd.org) Received: (from scottl@localhost) by svn.freebsd.org (8.14.7/8.14.5/Submit) id r570MdG8065048; Fri, 7 Jun 2013 00:22:39 GMT (envelope-from scottl@svn.freebsd.org) Message-Id: <201306070022.r570MdG8065048@svn.freebsd.org> From: Scott Long Date: Fri, 7 Jun 2013 00:22:39 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r251479 - in head/sys/cam: . scsi X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Jun 2013 00:22:40 -0000 Author: scottl Date: Fri Jun 7 00:22:38 2013 New Revision: 251479 URL: http://svnweb.freebsd.org/changeset/base/251479 Log: 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 Modified: head/sys/cam/cam_periph.c head/sys/cam/scsi/scsi_pass.c head/sys/cam/scsi/scsi_sg.c head/sys/cam/scsi/scsi_target.c Modified: head/sys/cam/cam_periph.c ============================================================================== --- head/sys/cam/cam_periph.c Thu Jun 6 23:21:41 2013 (r251478) +++ head/sys/cam/cam_periph.c Fri Jun 7 00:22:38 2013 (r251479) @@ -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 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 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 } - /* 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, stru 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; } Modified: head/sys/cam/scsi/scsi_pass.c ============================================================================== --- head/sys/cam/scsi/scsi_pass.c Thu Jun 6 23:21:41 2013 (r251478) +++ head/sys/cam/scsi/scsi_pass.c Fri Jun 7 00:22:38 2013 (r251479) @@ -668,12 +668,11 @@ passsendccb(struct cam_periph *periph, u { 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, u 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, u */ 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, u 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; Modified: head/sys/cam/scsi/scsi_sg.c ============================================================================== --- head/sys/cam/scsi/scsi_sg.c Thu Jun 6 23:21:41 2013 (r251478) +++ head/sys/cam/scsi/scsi_sg.c Fri Jun 7 00:22:38 2013 (r251479) @@ -946,25 +946,23 @@ sgsendccb(struct cam_periph *periph, uni { 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)); - - /* - * 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; - } + 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. + * 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, uni SF_RETRY_UA, softc->device_stats); - if (need_unmap) - cam_periph_unmapmem(ccb, &mapinfo); + cam_periph_unmapmem(ccb, &mapinfo); return (error); } Modified: head/sys/cam/scsi/scsi_target.c ============================================================================== --- head/sys/cam/scsi/scsi_target.c Thu Jun 6 23:21:41 2013 (r251478) +++ head/sys/cam/scsi/scsi_target.c Fri Jun 7 00:22:38 2013 (r251479) @@ -726,21 +726,8 @@ targsendccb(struct targ_softc *softc, un 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);