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
[-- Attachment #1 --]
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?
[-- Attachment #2 --]
>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
[-- Attachment #3 --]
>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
[-- Attachment #4 --]
>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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170307010204.GA3611>