From owner-freebsd-scsi Fri Nov 29 11:38:38 2002 Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 40B8B37B401 for ; Fri, 29 Nov 2002 11:38:32 -0800 (PST) Received: from comp.chem.msu.su (comp-ext.chem.msu.su [158.250.32.157]) by mx1.FreeBSD.org (Postfix) with ESMTP id 11F5543E9C for ; Fri, 29 Nov 2002 11:38:26 -0800 (PST) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (localhost [127.0.0.1]) by comp.chem.msu.su (8.12.3/8.12.3) with ESMTP id gATJcJ9K041684; Fri, 29 Nov 2002 22:38:19 +0300 (MSK) (envelope-from yar@comp.chem.msu.su) Received: (from yar@localhost) by comp.chem.msu.su (8.12.3/8.12.3/Submit) id gATJcImu041683; Fri, 29 Nov 2002 22:38:18 +0300 (MSK) Date: Fri, 29 Nov 2002 22:38:17 +0300 From: Yar Tikhiy To: Nate Lawson Cc: freebsd-scsi@freebsd.org Subject: Re: {da,sa,...}open bug? Message-ID: <20021129223817.D34288@comp.chem.msu.su> References: <20021125134302.D14452@comp.chem.msu.su> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: ; from nate@root.org on Mon, Nov 25, 2002 at 12:10:14PM -0800 Sender: owner-freebsd-scsi@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org On Mon, Nov 25, 2002 at 12:10:14PM -0800, Nate Lawson wrote: > On Mon, 25 Nov 2002, Yar Tikhiy wrote: > > > While preparing the fix, I noticed an additional couple of oddities. > > First, files under sys/cam/scsi are inconsistent as to the order of > > calling cam_periph_release() and cam_periph_unlock(): Some of them > > will call cam_periph_release() first, and the others will call it second. > > Then, there's a number of places in the code where cam_periph_unlock() > > won't be called before return on a cam_periph_acquire() error, though > > the "periph" has been locked. > > I think this should be fixed. Please submit a patch for this. Here it is. It a) reorders unlock()'s and release()'s where necessary, b) adds missing unlock()'s, and finally c) changes "return(error)" to "return(0)" where "error" will be always 0. The latter is essentially a style fix, but it is important WRT the discussed necessity to release a peripheral on errors. Having no "if (error) cam_periph_release(periph)" before such returns would be confusing. To Nate: If the patch looks good to you, please just say OK, and I'll do the dirty work of obtaining the high approval and committing. -- Yar Index: scsi_cd.c =================================================================== RCS file: /home/ncvs/src/sys/cam/scsi/scsi_cd.c,v retrieving revision 1.68 diff -u -r1.68 scsi_cd.c --- scsi_cd.c 23 Nov 2002 22:51:50 -0000 1.68 +++ scsi_cd.c 29 Nov 2002 19:16:19 -0000 @@ -903,16 +903,18 @@ splx(s); - if (cam_periph_acquire(periph) != CAM_REQ_CMP) + if (cam_periph_acquire(periph) != CAM_REQ_CMP) { + cam_periph_unlock(periph); return(ENXIO); + } cdprevent(periph, PR_PREVENT); /* find out the size */ if ((error = cdsize(dev, &size)) != 0) { cdprevent(periph, PR_ALLOW); - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return(error); } @@ -931,7 +933,7 @@ CAM_DEBUG(periph->path, CAM_DEBUG_TRACE, ("leaving cdopen\n")); - return (error); + return (0); } static int @@ -959,8 +961,8 @@ */ softc->device_stats.flags |= DEVSTAT_BS_UNAVAILABLE; - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return (0); } Index: scsi_ch.c =================================================================== RCS file: /home/ncvs/src/sys/cam/scsi/scsi_ch.c,v retrieving revision 1.31 diff -u -r1.31 scsi_ch.c --- scsi_ch.c 28 Sep 2002 17:14:05 -0000 1.31 +++ scsi_ch.c 29 Nov 2002 19:16:19 -0000 @@ -438,8 +438,10 @@ splx(s); if ((softc->flags & CH_FLAG_OPEN) == 0) { - if (cam_periph_acquire(periph) != CAM_REQ_CMP) + if (cam_periph_acquire(periph) != CAM_REQ_CMP) { + cam_periph_unlock(periph); return(ENXIO); + } softc->flags |= CH_FLAG_OPEN; } @@ -448,14 +450,14 @@ */ if ((error = chgetparams(periph)) != 0) { softc->flags &= ~CH_FLAG_OPEN; - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return(error); } cam_periph_unlock(periph); - return(error); + return(0); } static int @@ -478,8 +480,8 @@ softc->flags &= ~CH_FLAG_OPEN; - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return(0); } Index: scsi_da.c =================================================================== RCS file: /home/ncvs/src/sys/cam/scsi/scsi_da.c,v retrieving revision 1.116 diff -u -r1.116 scsi_da.c --- scsi_da.c 29 Nov 2002 15:40:10 -0000 1.116 +++ scsi_da.c 29 Nov 2002 19:16:20 -0000 @@ -545,8 +545,10 @@ if ((error = cam_periph_lock(periph, PRIBIO|PCATCH)) != 0) return (error); /* error code from tsleep */ - if (cam_periph_acquire(periph) != CAM_REQ_CMP) + if (cam_periph_acquire(periph) != CAM_REQ_CMP) { + cam_periph_unlock(periph); return(ENXIO); + } softc->flags |= DA_FLAG_OPEN; if ((softc->flags & DA_FLAG_PACK_INVALID) != 0) { @@ -696,8 +698,8 @@ } softc->flags &= ~DA_FLAG_OPEN; - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return (0); } Index: scsi_pass.c =================================================================== RCS file: /home/ncvs/src/sys/cam/scsi/scsi_pass.c,v retrieving revision 1.34 diff -u -r1.34 scsi_pass.c --- scsi_pass.c 15 Aug 2002 20:54:03 -0000 1.34 +++ scsi_pass.c 29 Nov 2002 19:16:20 -0000 @@ -384,14 +384,16 @@ splx(s); if ((softc->flags & PASS_FLAG_OPEN) == 0) { - if (cam_periph_acquire(periph) != CAM_REQ_CMP) + if (cam_periph_acquire(periph) != CAM_REQ_CMP) { + cam_periph_unlock(periph); return(ENXIO); + } softc->flags |= PASS_FLAG_OPEN; } cam_periph_unlock(periph); - return (error); + return (0); } static int @@ -412,8 +414,8 @@ softc->flags &= ~PASS_FLAG_OPEN; - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return (0); } Index: scsi_pt.c =================================================================== RCS file: /home/ncvs/src/sys/cam/scsi/scsi_pt.c,v retrieving revision 1.33 diff -u -r1.33 scsi_pt.c --- scsi_pt.c 15 Aug 2002 20:54:03 -0000 1.33 +++ scsi_pt.c 29 Nov 2002 19:16:20 -0000 @@ -198,8 +198,8 @@ return (error); /* error code from tsleep */ softc->flags &= ~PT_FLAG_OPEN; - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return (0); } Index: scsi_sa.c =================================================================== RCS file: /home/ncvs/src/sys/cam/scsi/scsi_sa.c,v retrieving revision 1.84 diff -u -r1.84 scsi_sa.c --- scsi_sa.c 14 Nov 2002 05:03:11 -0000 1.84 +++ scsi_sa.c 29 Nov 2002 19:16:20 -0000 @@ -640,8 +640,8 @@ if ((softc->flags & SA_FLAG_TAPE_MOUNTED) == 0) sareservereleaseunit(periph, FALSE); - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return (error); } Index: scsi_ses.c =================================================================== RCS file: /home/ncvs/src/sys/cam/scsi/scsi_ses.c,v retrieving revision 1.23 diff -u -r1.23 scsi_ses.c --- scsi_ses.c 9 Nov 2002 12:55:04 -0000 1.23 +++ scsi_ses.c 29 Nov 2002 19:16:21 -0000 @@ -488,8 +488,8 @@ softc->ses_flags &= ~SES_FLAG_OPEN; - cam_periph_unlock(periph); cam_periph_release(periph); + cam_periph_unlock(periph); return (0); } To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-scsi" in the body of the message