Date: Mon, 6 Mar 2017 17:02:04 -0800 From: Mark Johnston <markj@FreeBSD.org> To: Mark Millard <markmi@dsl-only.net> Cc: FreeBSD PowerPC ML <freebsd-ppc@freebsd.org>, Justin Hibbits <chmeeedalf@gmail.com>, Nathan Whitehorn <nwhitehorn@freebsd.org> Subject: Re: powerpc64 head -r314687 (PowerMac G5 so-called "Quad Core", clang based): CAM status: Command timeout (always?) Message-ID: <20170307010204.GA3611@wkstn-mjohnston.west.isilon.com> In-Reply-To: <E67A6606-941D-4F00-993D-4347C2A1D332@dsl-only.net> References: <98A62E0D-C2A0-40B1-AE6D-5810906208AE@dsl-only.net> <4C78F6AA-5ABD-4445-B5EF-4E6778CE36FE@dsl-only.net> <20170306164341.GA83069@wkstn-mjohnston.west.isilon.com> <466C25ED-0A70-4988-9BB1-3B43BD031E5E@dsl-only.net> <E67A6606-941D-4F00-993D-4347C2A1D332@dsl-only.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--0F1p//8PRICkK4MW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Mar 06, 2017 at 02:01:06PM -0800, Mark Millard wrote: > [scsi_pass.c -r314624 is the problem file vintage of the two files.] > > On 2017-Mar-6, at 10:36 AM, Mark Millard <markmi at dsl-only.net> wrote: > > > On 2017-Mar-6, at 8:43 AM, Mark Johnston <markj at FreeBSD.org> wrote: > > > >> On Mon, Mar 06, 2017 at 02:05:39AM -0800, Mark Millard wrote: > >>> On 2017-Mar-6, at 1:37 AM, Mark Millard <markmi at dsl-only.net> wrote: > >>> [...] > >>> Yep: reverting the two files allowed the PowerMac G5 so-called > >>> "Quad Core" to boot fully and I could log in. > >> > >> Do you have a full dmesg of the failed boot? Am I correct in thinking > >> that the boot failed before making it to user mode? > > > > . . . > >> If so I'm rather > >> puzzled, as the change should only affect userland applications. > >> Specifically, it modified a couple of ioctl handlers. > >> > >>> > >>> It appears that if such powerpc64 machines are to stay bootable > >>> then other things need to be cleaned up before the two updated > >>> files from -r314624 should be used. > >>> > >>> Should the 2 files be reverted until other things are cleaned up? > >> > >> I don't mind reverting the change, but my suspicion is that it uncovered > >> a problem rather than introducing it. If you're willing to narrow things > >> down a bit, could you try booting with one of the file modifications and > >> not the other? They are independent. > > > > In a while I'll try each of the files individually, one old, one modern > > each time. > > scsi_pass.c -r314624 (new) and cam_xpt.c -r314283 (old): fails. > > cam_xpt.c -r314624 (new) and scsi_pass.c -r308451 (old) : works fine so far. > > Prior results: > > cam_xpt.c and scsi_pass.c both being -r314624 (both new): fails > > cam_xpt.c -r314283 and scsi_pass.c -r308451 (both old): works fine. Thank you. I'm still failing to see how the change is connected with the symptoms you're seeing. Are you testing with a kernel that has INVARIANTS and WITNESS configured? I've broken up the scsi_pass.c change into several patches. They are sequential; can you try testing the result of each patch in the series? --0F1p//8PRICkK4MW Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Check-for-CAM_UNLOCKED-in-CAMIOCOMMAND.patch" >From c15f8efec3379966e48d20aa813cc13b6088f0d7 Mon Sep 17 00:00:00 2001 From: Mark Johnston <mjohnston@isilon.com> Date: Mon, 6 Mar 2017 16:48:48 -0800 Subject: [PATCH 1/3] Check for CAM_UNLOCKED in CAMIOCOMMAND. --- sys/cam/scsi/scsi_pass.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sys/cam/scsi/scsi_pass.c b/sys/cam/scsi/scsi_pass.c index 93e45d0b3706..d8bf615fd7e3 100644 --- a/sys/cam/scsi/scsi_pass.c +++ b/sys/cam/scsi/scsi_pass.c @@ -1782,6 +1782,11 @@ passdoioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flag, struct thread inccb->csio.bio = NULL; #endif + if (inccb->ccb_h.flags & CAM_UNLOCKED) { + error = EINVAL; + break; + } + /* * Some CCB types, like scan bus and scan lun can only go * through the transport layer device. -- 2.11.1 --0F1p//8PRICkK4MW Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0002-Check-for-CAM_UNLOCKED-in-CAMIOQUEUE.patch" >From 4727b70d32965b23af2cbd16cecb85422059fe78 Mon Sep 17 00:00:00 2001 From: Mark Johnston <mjohnston@isilon.com> Date: Mon, 6 Mar 2017 16:49:40 -0800 Subject: [PATCH 2/3] Check for CAM_UNLOCKED in CAMIOQUEUE. --- sys/cam/scsi/scsi_pass.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sys/cam/scsi/scsi_pass.c b/sys/cam/scsi/scsi_pass.c index d8bf615fd7e3..ab9b72106816 100644 --- a/sys/cam/scsi/scsi_pass.c +++ b/sys/cam/scsi/scsi_pass.c @@ -1889,6 +1889,13 @@ passdoioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flag, struct thread ccb->csio.bio = NULL; #endif + if (ccb->ccb_h.flags & CAM_UNLOCKED) { + error = EINVAL; + uma_zfree(softc->pass_zone, io_req); + cam_periph_lock(periph); + break; + } + if (ccb->ccb_h.flags & CAM_CDB_POINTER) { if (ccb->csio.cdb_len > IOCDBLEN) { error = EINVAL; -- 2.11.1 --0F1p//8PRICkK4MW Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0003-Fix-error-handling-for-CAMIOQUEUE.patch" >From 0655d0ec6a9ecac0150719dbec24cede7464c924 Mon Sep 17 00:00:00 2001 From: Mark Johnston <mjohnston@isilon.com> Date: Mon, 6 Mar 2017 16:58:19 -0800 Subject: [PATCH 3/3] Fix error handling for CAMIOQUEUE. --- sys/cam/scsi/scsi_pass.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/sys/cam/scsi/scsi_pass.c b/sys/cam/scsi/scsi_pass.c index ab9b72106816..aa13a01d1156 100644 --- a/sys/cam/scsi/scsi_pass.c +++ b/sys/cam/scsi/scsi_pass.c @@ -1880,9 +1880,7 @@ passdoioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flag, struct thread xpt_print(periph->path, "Copy of user CCB %p to " "kernel address %p failed with error %d\n", *user_ccb, ccb, error); - uma_zfree(softc->pass_zone, io_req); - cam_periph_lock(periph); - break; + goto camioqueue_error; } #if defined(BUF_TRACKING) || defined(FULL_BUF_TRACKING) if (ccb->ccb_h.func_code == XPT_SCSI_IO) @@ -1891,20 +1889,18 @@ passdoioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flag, struct thread if (ccb->ccb_h.flags & CAM_UNLOCKED) { error = EINVAL; - uma_zfree(softc->pass_zone, io_req); - cam_periph_lock(periph); - break; + goto camioqueue_error; } if (ccb->ccb_h.flags & CAM_CDB_POINTER) { if (ccb->csio.cdb_len > IOCDBLEN) { error = EINVAL; - break; + goto camioqueue_error; } error = copyin(ccb->csio.cdb_io.cdb_ptr, ccb->csio.cdb_io.cdb_bytes, ccb->csio.cdb_len); - if (error) - break; + if (error != 0) + goto camioqueue_error; ccb->ccb_h.flags &= ~CAM_CDB_POINTER; } @@ -1916,10 +1912,8 @@ passdoioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flag, struct thread xpt_print(periph->path, "CCB function code %#x is " "restricted to the XPT device\n", ccb->ccb_h.func_code); - uma_zfree(softc->pass_zone, io_req); - cam_periph_lock(periph); error = ENODEV; - break; + goto camioqueue_error; } /* @@ -1965,11 +1959,8 @@ passdoioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flag, struct thread || (fc == XPT_SMP_IO) || (fc == XPT_DEV_MATCH) || (fc == XPT_DEV_ADVINFO)) { error = passmemsetup(periph, io_req); - if (error != 0) { - uma_zfree(softc->pass_zone, io_req); - cam_periph_lock(periph); - break; - } + if (error != 0) + goto camioqueue_error; } else io_req->mapinfo.num_bufs_used = 0; @@ -2014,6 +2005,11 @@ passdoioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flag, struct thread TAILQ_INSERT_TAIL(&softc->done_queue, io_req, links); } break; + +camioqueue_error: + uma_zfree(softc->pass_zone, io_req); + cam_periph_lock(periph); + break; } case CAMIOGET: { -- 2.11.1 --0F1p//8PRICkK4MW--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170307010204.GA3611>