Date: Sun, 13 Oct 2002 23:47:58 -0600 From: "Kenneth D. Merry" <ken@kdm.org> To: Bruce Evans <bde@zeta.org.au> Cc: "M. Warner Losh" <imp@bsdimp.com>, johan@FreeBSD.ORG, nsayer@quack.kfu.com, freebsd-scsi@FreeBSD.ORG, sos@FreeBSD.ORG Subject: Re: kern/15608: acd0 / cd0 give inconsistent errors on empty tray open() Message-ID: <20021013234758.A75169@panzer.kdm.org> In-Reply-To: <20021001142616.M354-100000@gamplex.bde.org>; from bde@zeta.org.au on Tue, Oct 01, 2002 at 03:06:21PM %2B1000 References: <20020829223429.A62384@panzer.kdm.org> <20021001142616.M354-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Tue, Oct 01, 2002 at 15:06:21 +1000, Bruce Evans wrote:
> [Lot's quoted since this is a reply to old mail]
>
> Long ago, on Thu, 29 Aug 2002, Kenneth D. Merry wrote:
>
> > On Thu, Aug 29, 2002 at 21:52:27 +1000, Bruce Evans wrote:
> > > On Mon, 26 Aug 2002, Kenneth D. Merry wrote:
> > > > On Sun, Aug 25, 2002 at 17:07:32 +1000, Bruce Evans wrote:
> > >
> > > > > [...]
> > > > > There is a CDIOCCLOSE which seems to be supported by acd and by some
> > > > > unmaintained cdrom drivers by not by the scsi cdrom driver.
> > > >
> > > > Ahh, I missed that.
> > > > > [...]
> > > > Well, here is a first pass at it. Let me know what you think.
>
> I finally configured atapicam so that I could test this, but I seem to have
> deleted the mail with the patch. Please resend the patch.
Sorry for the delay on this end, lots going on. I've attached the patch.
I think it still works as before, but I've discovered that Poul-Henning
broke disklabel support in the cd(4) driver. I need to talk to him about
that. (It isn't directly related to whether this patch is the right thing
to do.)
> > > > > dsopen() has similar issues. It attempts to read MBRs and disk labels
> > > > > and can probably return EIO for read errors when there is no media or
> > > > > bad media. One reason why the fd driver doesn't use the slice layer
> > > > > is that I never got this working well enough for floppies. It is hard
> > > > > to issue formatting ioctls when the open fails because the MBR is
> > > > > unreadable.
> > > >
> > > > FWIW, if the slice code is setup to attempt to read labels off the disk, but
> > > > there is no media in the drive (i.e. sector size and sectors per unit are
> > > > 0, or it could be because p_size for the first partition is 0), it panics.
> > > > (DSO_COMPATLABEL is set, and DSO_NOLABELS is cleared)
> > > >
> > > > If the slice code is setup not to attempt to read labels off the disk, it
> > > > doesn't panic at least.
> > >
> > > Hmm. I never committed the patch that makes dsopen() return EINVAL
> > > if the sector size is 0. (We added an early return if the sector size
> > > is not a multiple of DEV_BSIZE some time ago.) But these early returns
> > > of an error are not quite right, since they cause the whole open() to
> > > fail, but dsopen() is really only about the slice+label parts of opening
> > > disks.
>
> This is now "fixed" in diskopen() by dividing by the sector size before
> dsopen() can validate it. When the drive tray is open, division by 0
> only happens if there have been no previous successful opens, so something
> is apparently remembering the label for too long. These are some of the
> bugs introduced in subr_disk.c rev.1.59 (the others are lossage of passing
> the d_type, d_typename, d_packname and d_flags fields from the driver).
Yeah, it's unfortunate we can't pass that information now.
> > > > Behavior of things like dd(1) is different, obviously, because it no longer
> > > > fails in open() with an empty drive. e.g., new behavior is:
> > > >
> > > > ================
> > > > {nargothrond:/usr/home/ken:2:0} dd if=/dev/cd0c of=/dev/null bs=2k
> > > > dd: /dev/cd0c: Device not configured
> > > > 0+0 records in
> > > > 0+0 records out
> > > > 0 bytes transferred in 0.004522 secs (0 bytes/sec)
> > > > ================
> > >
> > > Shouldn't the error be EIO now? Hmm, POSIX just made ENXIO standard for
> > > read(). From POSIX.1-2001-draft7:
> > >
> > > % 36723 [ENXIO] A request was made of a nonexistent device, or the request was outside the
> > > % 36724 capabilities of the device.
> > > % ...
> > > % 36845 * The [ENXIO] optional error condition is added.
> > >
> > > Similarly for write(). ENXIO is very reasonable for i/o an open device that
> > > went away.
> >
> > Cool. The error returned will be whatever the error recovery code decides
> > is appropriate. In the case of "medium not present" errors, that means
> > ENXIO. (see asc_table[] in sys/cam/scsi/scsi_all.c )
>
> So this thread is now about errors in read(): acd gives EIO but cd will give
> ENXIO. (cd still gives ENXIO in open() in -current.)
I suppose so. The cd(4) driver will give varying errors, depending on what
the drive tells it. If it's a "medium not present" type error, it'll
return ENXIO. Otherwise, it could be most anything.
> > > > cdcontrol seems to get a better idea of what is going on when open() fails:
> > > >
> > > > ================
> > > > {gondolin:/usr/home/ken:7:1} cdcontrol
> > > > cdcontrol: no CD device name specified, defaulting to /dev/cd0c
> > > > Compact Disc Control utility, version 2.0
> > > > Type `?' for command list
> > > >
> > > > cdcontrol> info
> > > > cdcontrol: no disc in drive /dev/cd0c
> > > > cdcontrol> quit
> > > > ================
> > >
> > > It also prints this if there is no cd0 device but there is a cd0c inode :-).
> > > The comment before it prints this says that ENXIO is overloaded, but here
> > > it is too overloaded to work.
> >
> > Ahh.
> >
> > > > When the ioctl fails instead of the open, it isn't quite as clear about
> > > > what is going on:
> > > >
> > > > ================
> > > > {nargothrond:/usr/home/ken:3:1} cdcontrol
> > > > cdcontrol: no CD device name specified, defaulting to /dev/cd0c
> > > > Compact Disc Control utility, version 2.0
> > > > Type `?' for command list
> > > >
> > > > cdcontrol> info
> > > > cdcontrol: getting toc header: Device not configured
> > > > cdcontrol: Device not configured
> > > > cdcontrol> quit
> > > > ================
> > >
> > > Here it could more reasonably say that there is no disc in the drive, since
> > > there must at least have been a drive to get that far. Similarly for ENXIO
> > > in read().
> >
> > Does the acd(4) driver produce the same behavior?
>
> acd gives almost the second behaviour: a read error "getting toc header".
> The read error is EIO instead of ENXIO.
Ahh.
> > > I didn't try the patch or look closely at it. My only SCSI cdrom is 8 years
> > > old hasn't been able to read disks for 2-3 years.
> >
> > Bummer. I think you should be able to try it out with an ATAPI CDROM
> > drive if you turn on options ATAPICAM. (FWIW, I don't have any ATA or
> > ATAPI peripherals at home, except for two disk drives that are mostly dead
> > and haven't been turned on in 7 or 8 years. :)
>
> Thanks for the hint. BTW, atapicam has the interesting property of living
> alongside atapi. I get devices afd0/da0, cd0/acd0 and cd1/acd1 here. They
> all work, at least non-concurrently. I haven't tried concurrent opens and
> expect they would cause problems, especially for writable devices.
Yeah, we don't have locking there. There also isn't any locking to prevent
concurrent access via multiple CAM peripheral drivers. (e.g. you can talk
to your CDROM drive via pass(4) and cd(4) at the same time. Many time this
is a feature.)
Ken
--
Kenneth Merry
ken@kdm.org
--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="scsi_cd.c.20021013"
==== //depot/FreeBSD-ken/src/sys/cam/scsi/scsi_cd.c#27 - /usr/home/ken/perforce/FreeBSD-ken/src/sys/cam/scsi/scsi_cd.c ====
--- /tmp/tmp.291.0 Sun Oct 13 23:30:46 2002
+++ /usr/home/ken/perforce/FreeBSD-ken/src/sys/cam/scsi/scsi_cd.c Sun Oct 13 23:29:48 2002
@@ -1,6 +1,6 @@
/*
* Copyright (c) 1997 Justin T. Gibbs.
- * Copyright (c) 1997, 1998, 1999, 2000, 2001 Kenneth D. Merry.
+ * Copyright (c) 1997, 1998, 1999, 2000, 2001, 2002 Kenneth D. Merry.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -94,7 +94,8 @@
CD_FLAG_CHANGER = 0x040,
CD_FLAG_ACTIVE = 0x080,
CD_FLAG_SCHED_ON_COMP = 0x100,
- CD_FLAG_RETRY_UA = 0x200
+ CD_FLAG_RETRY_UA = 0x200,
+ CD_FLAG_VALID_MEDIA = 0x400
} cd_flags;
typedef enum {
@@ -205,7 +206,8 @@
static int cderror(union ccb *ccb, u_int32_t cam_flags,
u_int32_t sense_flags);
static void cdprevent(struct cam_periph *periph, int action);
-static int cdsize(dev_t dev, u_int32_t *size);
+static int cdcheckmedia(struct cam_periph *periph);
+static int cdsize(struct cam_periph *periph, u_int32_t *size);
static int cdreadtoc(struct cam_periph *periph, u_int32_t mode,
u_int32_t start, struct cd_toc_entry *data,
u_int32_t len);
@@ -229,7 +231,7 @@
u_int32_t etrack, u_int32_t eindex);
static int cdpause(struct cam_periph *periph, u_int32_t go);
static int cdstopunit(struct cam_periph *periph, u_int32_t eject);
-static int cdstartunit(struct cam_periph *periph);
+static int cdstartunit(struct cam_periph *periph, int load);
static int cdreportkey(struct cam_periph *periph,
struct dvd_authinfo *authinfo);
static int cdsendkey(struct cam_periph *periph,
@@ -874,7 +876,6 @@
{
struct cam_periph *periph;
struct cd_softc *softc;
- u_int32_t size;
int error;
int s;
@@ -903,26 +904,12 @@
if (cam_periph_acquire(periph) != CAM_REQ_CMP)
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);
- return(error);
- }
-
/*
- * We unconditionally (re)set the blocksize each time the
- * CD device is opened. This is because the CD can change,
- * and therefore the blocksize might change.
- * XXX problems here if some slice or partition is still
- * open with the old size?
+ * Check for media, and set the appropriate flags. We don't bail
+ * if we don't have media, but then we don't allow anything but the
+ * CDIOCEJECT/CDIOCCLOSE ioctls if there is no media.
*/
- if ((softc->device_stats.flags & DEVSTAT_BS_UNAVAILABLE) != 0)
- softc->device_stats.flags &= ~DEVSTAT_BS_UNAVAILABLE;
- softc->device_stats.block_size = softc->params.blksize;
+ cdcheckmedia(periph);
cam_periph_unlock(periph);
@@ -1332,6 +1319,21 @@
}
/*
+ * If we don't have valid media, look for it before trying to
+ * schedule the I/O.
+ */
+ if ((softc->flags & CD_FLAG_VALID_MEDIA) == 0) {
+ int error;
+
+ error = cdcheckmedia(periph);
+ if (error != 0) {
+ splx(s);
+ biofinish(bp, NULL, error);
+ return;
+ }
+ }
+
+ /*
* Place it in the queue of disk activities for this disk
*/
bioqdisksort(&softc->bio_queue, bp);
@@ -1768,6 +1770,20 @@
if (error != 0)
return(error);
+ /*
+ * If we don't have media loaded, check for it. If we still don't
+ * have media loaded, we can only do a load or eject.
+ */
+ if (((softc->flags & CD_FLAG_VALID_MEDIA) == 0)
+ && ((cmd != CDIOCCLOSE)
+ && (cmd != CDIOCEJECT))) {
+ error = cdcheckmedia(periph);
+ if (error != 0) {
+ cam_periph_unlock(periph);
+ return (error);
+ }
+ }
+
switch (cmd) {
case DIOCGMEDIASIZE:
@@ -2353,7 +2369,10 @@
error = cdpause(periph, 0);
break;
case CDIOCSTART:
- error = cdstartunit(periph);
+ error = cdstartunit(periph, 0);
+ break;
+ case CDIOCCLOSE:
+ error = cdstartunit(periph, 1);
break;
case CDIOCSTOP:
error = cdstopunit(periph, 0);
@@ -2457,18 +2476,49 @@
}
static int
-cdsize(dev_t dev, u_int32_t *size)
+cdcheckmedia(struct cam_periph *periph)
+{
+ struct cd_softc *softc;
+ u_int32_t size;
+ int error;
+
+ softc = (struct cd_softc *)periph->softc;
+
+ cdprevent(periph, PR_PREVENT);
+
+ /*
+ * Get the disc size and block size. If we can't get it, we don't
+ * have media, most likely. So we clear the valid media flag.
+ */
+ if ((error = cdsize(periph, &size)) != 0) {
+ softc->flags &= ~CD_FLAG_VALID_MEDIA;
+ cdprevent(periph, PR_ALLOW);
+ return (error);
+ } else
+ softc->flags |= CD_FLAG_VALID_MEDIA;
+
+ /*
+ * We unconditionally (re)set the blocksize each time the
+ * CD device is opened. This is because the CD can change,
+ * and therefore the blocksize might change.
+ * XXX problems here if some slice or partition is still
+ * open with the old size?
+ */
+ if ((softc->device_stats.flags & DEVSTAT_BS_UNAVAILABLE) != 0)
+ softc->device_stats.flags &= ~DEVSTAT_BS_UNAVAILABLE;
+ softc->device_stats.block_size = softc->params.blksize;
+
+ return (error);
+}
+
+static int
+cdsize(struct cam_periph *periph, u_int32_t *size)
{
- struct cam_periph *periph;
struct cd_softc *softc;
union ccb *ccb;
struct scsi_read_capacity_data *rcap_buf;
int error;
- periph = (struct cam_periph *)dev->si_drv1;
- if (periph == NULL)
- return (ENXIO);
-
CAM_DEBUG(periph->path, CAM_DEBUG_TRACE, ("entering cdsize\n"));
softc = (struct cd_softc *)periph->softc;
@@ -2900,7 +2950,7 @@
}
static int
-cdstartunit(struct cam_periph *periph)
+cdstartunit(struct cam_periph *periph, int load)
{
union ccb *ccb;
int error;
@@ -2914,7 +2964,7 @@
/* cbfcnp */ cddone,
/* tag_action */ MSG_SIMPLE_Q_TAG,
/* start */ TRUE,
- /* load_eject */ FALSE,
+ /* load_eject */ load,
/* immediate */ FALSE,
/* sense_len */ SSD_FULL_SIZE,
/* timeout */ 50000);
--mYCpIKhGyMATD0i+--
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-scsi" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021013234758.A75169>