Date: Sat, 19 Oct 2002 16:37:39 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: "Kenneth D. Merry" <ken@kdm.org> 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: <20021019152522.T1864-100000@gamplex.bde.org> In-Reply-To: <20021013234758.A75169@panzer.kdm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 13 Oct 2002, Kenneth D. Merry wrote:
> On Tue, Oct 01, 2002 at 15:06:21 +1000, Bruce Evans wrote:
> > [Lot's quoted since this is a reply to old mail]
[Lots not quoted since the context was so deep it was getting in the way]
> > 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.)
I noticed this too, and fixed it here (except in the GEOM case of course).
Patches at the end.
> > 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
This is just a bug (using the sector size initialized by the device open
without checking that the open succeeded. A nominal, garbage or previous
size is used. The cd driver doesn't initialize the size (except to 0)
until an open succeeds).
> > 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.
Actually, we can pass it, except in the GEOM case of course. The label is
still there in the !GEOM case and is slightly easier to initialize than
before (since the media size and sector size, which are the only
non-constant things, are passed in another way).
> > 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.
OK. I tried your patches a bit, but not very carefully. The most
noticeable difference was that CAM is much more verbose than acd about
missing media (it barfs 5 lines on the console).
Another interesting error turned up: one of my drives (a cheap cdrom drive)
mishandles a blank cd-rw blanked in the other drive (a not so cheap cd-rw
drive). At boot time, the drive keeps searching the cd-rw and FreeBSD
keeps tring to probe the drive ... until the disk is manually ejected;
this usually causes a null pointer panic. The kernel has acd and cd and
the misbehaviour is with and without GEOM. Both acd and cd seem to be
confused. acd seems to print more messages about resetting, but one of
the null pointer panics was in CAM.
---
The following patches mostly restore scsi_cd.c to the unaxed version
rev.1.59. Changes since then not related to axing are preserved. I
first tried to do this by calling the slice layer directly so that
the disk-mini layer is not involved. This mostly worked, but it was
too hard to get all the details right since some of them have moved
from the slice layer to the disk mini-layer.
These patches make the !GEOM case work like it used to provided you
also fix diskopen() to not divide by zero (a similar fix for dsopen()
is not needed provided the driver is careful not to pass a sector
size of 0 when open is succeeding). With GEOM, the only problm
aprt from missing support for labels seems to be that the 'a' and
'c' devices don't exist.
Some of the less obvious bugs fixed by this patch (at least in the
!GEOM case) are:
- bp->b_pblkno was always 0, so bioq_disksort() couldn't sort.
- device stats didn't show cd*. I'm not sure why and guess it was
due to missing infrastructure. Device stats showed acd* despite
acd* not being in `sysctl kern.disks`. acd of course sets up
everything itself since it doesn't use the disk mini-layer.
%%%
Index: scsi_cd.c
===================================================================
RCS file: /home/ncvs/src/sys/cam/scsi/scsi_cd.c,v
retrieving revision 1.67
diff -u -2 -r1.67 scsi_cd.c
--- scsi_cd.c 18 Oct 2002 22:03:38 -0000 1.67
+++ scsi_cd.c 19 Oct 2002 03:07:43 -0000
@@ -55,4 +55,6 @@
#include <sys/conf.h>
#include <sys/disk.h>
+#include <sys/disklabel.h>
+#include <sys/diskslice.h>
#include <sys/malloc.h>
#include <sys/cdio.h>
@@ -129,4 +131,5 @@
LIST_HEAD(, ccb_hdr) pending_ccbs;
struct cd_params params;
+ struct disk disk;
union ccb saved_ccb;
cd_quirks quirks;
@@ -136,6 +139,4 @@
int bufs_left;
struct cam_periph *periph;
- dev_t dev;
- eventhandler_tag clonetag;
};
@@ -208,4 +209,5 @@
static void cdprevent(struct cam_periph *periph, int action);
static int cdsize(dev_t dev, u_int32_t *size);
+static int cdfirsttrackisdata(struct cam_periph *periph);
static int cdreadtoc(struct cam_periph *periph, u_int32_t mode,
u_int32_t start, struct cd_toc_entry *data,
@@ -264,4 +266,5 @@
/* flags */ D_DISK,
};
+static struct cdevsw cddisk_cdevsw;
static int num_changers;
@@ -301,24 +304,4 @@
static void
-cdclone(void *arg, char *name, int namelen, dev_t *dev)
-{
- struct cd_softc *softc;
- const char *p;
- int l;
-
- softc = arg;
- p = devtoname(softc->dev);
- l = strlen(p);
- if (bcmp(name, p, l))
- return;
- if (name[l] != 'a' && name[l] != 'c')
- return;
- if (name[l + 1] != '\0')
- return;
- *dev = softc->dev;
- return;
-}
-
-static void
cdinit(void)
{
@@ -489,6 +472,7 @@
}
devstat_remove_entry(&softc->device_stats);
- destroy_dev(softc->dev);
- EVENTHANDLER_DEREGISTER(dev_clone, softc->clonetag);
+ if (softc->disk.d_dev) {
+ disk_destroy(softc->disk.d_dev);
+ }
free(softc, M_DEVBUF);
splx(s);
@@ -566,4 +550,5 @@
struct ccb_getdev *cgd;
caddr_t match;
+ dev_t disk_dev;
cgd = (struct ccb_getdev *)arg;
@@ -627,9 +612,11 @@
DEVSTAT_TYPE_CDROM | DEVSTAT_TYPE_IF_SCSI,
DEVSTAT_PRIORITY_CD);
- softc->dev = make_dev(&cd_cdevsw, periph->unit_number,
- UID_ROOT, GID_OPERATOR, 0640, "cd%d", periph->unit_number);
- softc->dev->si_drv1 = periph;
- softc->clonetag =
- EVENTHANDLER_REGISTER(dev_clone, cdclone, softc, 1000);
+ disk_dev = disk_create(periph->unit_number, &softc->disk,
+ DSO_ONESLICE | DSO_COMPATLABEL,
+ &cd_cdevsw, &cddisk_cdevsw);
+ disk_dev->si_drv1 = periph;
+ disk_dev->si_iosize_max = DFLTPHYS; /* XXX */
+ disk_dev->si_bsize_phys = 2048; /* XXX */
+ disk_dev->si_bsize_best = DFLTPHYS; /* XXX */
/*
@@ -876,6 +863,8 @@
cdopen(dev_t dev, int flags, int fmt, struct thread *td)
{
+ struct disklabel *label;
struct cam_periph *periph;
struct cd_softc *softc;
+ struct ccb_getdev cgd;
u_int32_t size;
int error;
@@ -912,10 +901,70 @@
if ((error = cdsize(dev, &size)) != 0) {
cdprevent(periph, PR_ALLOW);
- cam_periph_unlock(periph);
cam_periph_release(periph);
+ cam_periph_unlock(periph);
return(error);
}
/*
+ * If we get a non-zero return, revert back to not reading the
+ * label off the disk. The first track is likely audio, which
+ * won't have a disklabel.
+ */
+ if ((error = cdfirsttrackisdata(periph)) != 0) {
+ softc->disk.d_dsflags &= ~DSO_COMPATLABEL;
+ softc->disk.d_dsflags |= DSO_NOLABELS;
+ error = 0;
+ }
+
+ /*
+ * Build prototype label for whole disk if necessary. Be warrantedly
+ * chummy with the implementation because the implementation has been
+ * broken to not take all our label info as a parameter.
+ * Should take information about different data tracks from the
+ * TOC and put it in the partition table.
+ */
+ label = softc->disk.d_label;
+ if (label == NULL)
+ goto over; /* XXX no label -- GEOM lossage */
+ if (label->d_type)
+ goto over; /* XXX ugly goto to minimize diffs */
+ label->d_type = DTYPE_SCSI;
+
+ /*
+ * Grab the inquiry data to get the vendor and product names.
+ * Put them in the typename and packname for the label.
+ */
+ xpt_setup_ccb(&cgd.ccb_h, periph->path, /*priority*/ 1);
+ cgd.ccb_h.func_code = XPT_GDEV_TYPE;
+ xpt_action((union ccb *)&cgd);
+
+ strncpy(label->d_typename, cgd.inq_data.vendor,
+ min(SID_VENDOR_SIZE, sizeof(label->d_typename)));
+ strncpy(label->d_packname, cgd.inq_data.product,
+ min(SID_PRODUCT_SIZE, sizeof(label->d_packname)));
+
+#if 0 /* These are now passed as disk.d_sectorsize and disk.d_mediasize. */
+ label->d_secsize = softc->params.blksize;
+ label->d_secperunit = softc->params.disksize;
+#endif
+ label->d_flags = D_REMOVABLE;
+ /*
+ * Make partition 'a' cover the whole disk. This is a temporary
+ * compatibility hack. The 'a' partition should not exist, so
+ * the slice code won't create it. The slice code will make
+ * partition (RAW_PART + 'a') cover the whole disk and fill in
+ * some more defaults.
+ */
+ label->d_partitions[0].p_size = softc->params.disksize;
+ label->d_partitions[0].p_fstype = FS_OTHER;
+over:
+
+ softc->disk.d_sectorsize = softc->params.blksize;
+ softc->disk.d_mediasize = softc->params.blksize *
+ (off_t)softc->params.disksize;
+ softc->disk.d_fwsectors = 0;
+ softc->disk.d_fwheads = 0;
+
+ /*
* We unconditionally (re)set the blocksize each time the
* CD device is opened. This is because the CD can change,
@@ -955,4 +1004,11 @@
/*
+ * Unconditionally set the dsopen() flags back to their default
+ * state.
+ */
+ softc->disk.d_dsflags &= ~DSO_NOLABELS;
+ softc->disk.d_dsflags |= DSO_COMPATLABEL;
+
+ /*
* Since we're closing this CD, mark the blocksize as unavailable.
* It will be marked as available whence the CD is opened again.
@@ -960,6 +1016,6 @@
softc->device_stats.flags |= DEVSTAT_BS_UNAVAILABLE;
- cam_periph_unlock(periph);
cam_periph_release(periph);
+ cam_periph_unlock(periph);
return (0);
@@ -1350,6 +1406,4 @@
else
cdschedule(periph, /* priority */ 1);
-
- return;
}
@@ -1397,6 +1451,5 @@
/* byte2 */ 0,
/* minimum_cmd_size */ 10,
- /* lba */ bp->bio_blkno /
- (softc->params.blksize / DEV_BSIZE),
+ /* lba */ bp->bio_pblkno,
bp->bio_bcount / softc->params.blksize,
/* data_ptr */ bp->bio_data,
@@ -1527,9 +1580,5 @@
bp->bio_error = 0;
if (bp->bio_resid != 0) {
- /*
- * Short transfer ???
- * XXX: not sure this is correct for partial
- * transfers at EOM
- */
+ /* Short transfer ??? */
bp->bio_flags |= BIO_ERROR;
}
@@ -1774,12 +1823,4 @@
switch (cmd) {
- case DIOCGMEDIASIZE:
- *(off_t *)addr =
- (off_t)softc->params.blksize * softc->params.disksize;
- break;
- case DIOCGSECTORSIZE:
- *(u_int *)addr = softc->params.blksize;
- break;
-
case CDIOCPLAYTRACKS:
{
@@ -2400,5 +2441,5 @@
error = cdsendkey(periph, authinfo);
break;
- }
+ }
case DVDIOCREADSTRUCTURE: {
struct dvd_struct *dvdstruct;
@@ -2418,7 +2459,6 @@
CAM_DEBUG(periph->path, CAM_DEBUG_TRACE, ("leaving cdioctl\n"));
- if (error && bootverbose) {
+ if (error != 0 && bootverbose)
printf("scsi_cd.c::ioctl cmd=%08lx error=%d\n", cmd, error);
- }
return (error);
@@ -2519,4 +2559,78 @@
return (error);
+}
+
+/*
+ * The idea here is to try to figure out whether the first track is data or
+ * audio. If it is data, we can at least attempt to read a disklabel off
+ * the first sector of the disk. If it is audio, there won't be a
+ * disklabel.
+ *
+ * This routine returns 0 if the first track is data, and non-zero if there
+ * is an error or the first track is audio. (If either non-zero case, we
+ * should not attempt to read the disklabel.)
+ */
+static int
+cdfirsttrackisdata(struct cam_periph *periph)
+{
+ struct cdtocdata {
+ struct ioc_toc_header header;
+ struct cd_toc_entry entries[100];
+ };
+ struct cd_softc *softc;
+ struct ioc_toc_header *th;
+ struct cdtocdata *data;
+ int num_entries, i;
+ int error, first_track_audio;
+
+ error = 0;
+ first_track_audio = -1;
+
+ softc = (struct cd_softc *)periph->softc;
+
+ data = malloc(sizeof(struct cdtocdata), M_TEMP, M_WAITOK);
+
+ th = &data->header;
+ error = cdreadtoc(periph, 0, 0, (struct cd_toc_entry *)data,
+ sizeof(*data));
+
+ if (error)
+ goto bailout;
+
+ if (softc->quirks & CD_Q_BCD_TRACKS) {
+ /* we are going to have to convert the BCD
+ * encoding on the cd to what is expected
+ */
+ th->starting_track =
+ bcd2bin(th->starting_track);
+ th->ending_track = bcd2bin(th->ending_track);
+ }
+ th->len = scsi_2btoul((u_int8_t *)&th->len);
+
+ if ((th->len - 2) > 0)
+ num_entries = (th->len - 2) / sizeof(struct cd_toc_entry);
+ else
+ num_entries = 0;
+
+ for (i = 0; i < num_entries; i++) {
+ if (data->entries[i].track == th->starting_track) {
+ if (data->entries[i].control & 0x4)
+ first_track_audio = 0;
+ else
+ first_track_audio = 1;
+ break;
+ }
+ }
+
+ if (first_track_audio == -1)
+ error = ENOENT;
+ else if (first_track_audio == 1)
+ error = EINVAL;
+ else
+ error = 0;
+bailout:
+ free(data, M_TEMP);
+
+ return(error);
}
%%%
The patches are most easy to describe relative to rev.1.59:
% Index: scsi_cd.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/cam/scsi/scsi_cd.c,v
% retrieving revision 1.59
% diff -u -2 -r1.59 scsi_cd.c
% --- scsi_cd.c 15 Aug 2002 20:54:02 -0000 1.59
% +++ scsi_cd.c 19 Oct 2002 03:07:43 -0000
% @@ -25,5 +25,5 @@
% * SUCH DAMAGE.
% *
% - * $FreeBSD: src/sys/cam/scsi/scsi_cd.c,v 1.59 2002/08/15 20:54:02 njl Exp $
% + * $FreeBSD: src/sys/cam/scsi/scsi_cd.c,v 1.67 2002/10/18 22:03:38 njl Exp $
% */
% /*
% @@ -55,6 +55,9 @@
% #include <sys/conf.h>
% #include <sys/disk.h>
% +#include <sys/disklabel.h>
% +#include <sys/diskslice.h>
Needed now since <sys/disk.h> has been depolluted.
% #include <sys/malloc.h>
% #include <sys/cdio.h>
% +#include <sys/cdrio.h>
Part of the recent patch for speeds. Not related to axing.
% #include <sys/dvdio.h>
% #include <sys/devicestat.h>
% @@ -231,4 +234,6 @@
% static int cdstopunit(struct cam_periph *periph, u_int32_t eject);
% static int cdstartunit(struct cam_periph *periph);
% +static int cdsetspeed(struct cam_periph *periph,
% + u_int32_t rdspeed, u_int32_t wrspeed);
Part of the recent patch for speeds.
% static int cdreportkey(struct cam_periph *periph,
% struct dvd_authinfo *authinfo);
% @@ -246,8 +251,4 @@
% PERIPHDRIVER_DECLARE(cd, cddriver);
%
% -/* For 2.2-stable support */
% -#ifndef D_DISK
% -#define D_DISK 0
% -#endif
Approved axing :-).
% static struct cdevsw cd_cdevsw = {
% /* open */ cdopen,
% @@ -302,5 +303,5 @@
% static STAILQ_HEAD(changerlist, cdchanger) changerq;
%
% -void
% +static void
% cdinit(void)
% {
Lint fix.
% @@ -615,4 +616,7 @@
% &cd_cdevsw, &cddisk_cdevsw);
% disk_dev->si_drv1 = periph;
% + disk_dev->si_iosize_max = DFLTPHYS; /* XXX */
% + disk_dev->si_bsize_phys = 2048; /* XXX */
% + disk_dev->si_bsize_best = DFLTPHYS; /* XXX */
%
% /*
I want drivers to set these parameters instead of letting other parts
of the kernel default them. The XXX's are because the values are no
better than the defaults.
% @@ -897,6 +901,6 @@
% if ((error = cdsize(dev, &size)) != 0) {
% cdprevent(periph, PR_ALLOW);
% - cam_periph_unlock(periph);
% cam_periph_release(periph);
% + cam_periph_unlock(periph);
% return(error);
% }
Half of a cosmetic patch I've been maintaining for 4 years. I like locks
to be released in the reverse order to which they are acquired.
% @@ -914,10 +918,15 @@
%
% /*
% - * Build prototype label for whole disk.
% + * Build prototype label for whole disk if necessary. Be warrantedly
% + * chummy with the implementation because the implementation has been
% + * broken to not take all our label info as a parameter.
% * Should take information about different data tracks from the
% * TOC and put it in the partition table.
% */
% - label = &softc->disk.d_label;
% - bzero(label, sizeof(*label));
% + label = softc->disk.d_label;
% + if (label == NULL)
% + goto over; /* XXX no label -- GEOM lossage */
This avoids following a null pointer in the GEOM case.
% + if (label->d_type)
% + goto over; /* XXX ugly goto to minimize diffs */
This is just an optimization.
% label->d_type = DTYPE_SCSI;
%
% @@ -935,6 +944,8 @@
% min(SID_PRODUCT_SIZE, sizeof(label->d_packname)));
%
% +#if 0 /* These are now passed as disk.d_sectorsize and disk.d_mediasize. */
% label->d_secsize = softc->params.blksize;
% label->d_secperunit = softc->params.disksize;
% +#endif
Left the code just to show that not much is being changed.
% label->d_flags = D_REMOVABLE;
% /*
% @@ -945,7 +956,14 @@
% * some more defaults.
% */
% - label->d_partitions[0].p_size = label->d_secperunit;
% + label->d_partitions[0].p_size = softc->params.disksize;
Have to change this since d_secperunit is now not initialized until later.
% label->d_partitions[0].p_fstype = FS_OTHER;
% +over:
%
% + softc->disk.d_sectorsize = softc->params.blksize;
% + softc->disk.d_mediasize = softc->params.blksize *
% + (off_t)softc->params.disksize;
% + softc->disk.d_fwsectors = 0;
% + softc->disk.d_fwheads = 0;
% +
This is the only part of rev.1.59 that was really necessary.
% /*
% * We unconditionally (re)set the blocksize each time the
% @@ -998,6 +1016,6 @@
% softc->device_stats.flags |= DEVSTAT_BS_UNAVAILABLE;
%
% - cam_periph_unlock(periph);
% cam_periph_release(periph);
% + cam_periph_unlock(periph);
Other half of the cosmetic patch (see above).
%
% return (0);
% @@ -1388,6 +1406,4 @@
% else
% cdschedule(periph, /* priority */ 1);
% -
% - return;
% }
%
I tried not to see style bugs while writing this patch but saw this.
% @@ -2408,4 +2424,10 @@
% error = ENOTTY;
% break;
% + case CDRIOCREADSPEED:
% + error = cdsetspeed(periph, *(u_int32_t *)addr, CDR_MAX_SPEED);
% + break;
% + case CDRIOCWRITESPEED:
% + error = cdsetspeed(periph, CDR_MAX_SPEED, *(u_int32_t *)addr);
% + break;
% case DVDIOCSENDKEY:
% case DVDIOCREPORTKEY: {
Part of the recent patch for speeds.
% @@ -2437,4 +2459,6 @@
%
% CAM_DEBUG(periph->path, CAM_DEBUG_TRACE, ("leaving cdioctl\n"));
% + if (error != 0 && bootverbose)
% + printf("scsi_cd.c::ioctl cmd=%08lx error=%d\n", cmd, error);
%
% return (error);
Some debugging code (not mine) less some style bugs (excessive {}).
% @@ -3046,4 +3070,42 @@
% /* sense_len */ SSD_FULL_SIZE,
% /* timeout */ 50000);
% +
% + error = cdrunccb(ccb, cderror, /*cam_flags*/CAM_RETRY_SELTO,
% + /*sense_flags*/SF_RETRY_UA);
% +
% + xpt_release_ccb(ccb);
% +
% + return(error);
% +}
% +
% +static int
% +cdsetspeed(struct cam_periph *periph, u_int32_t rdspeed, u_int32_t wrspeed)
% +{
% + struct scsi_set_speed *scsi_cmd;
% + struct ccb_scsiio *csio;
% + union ccb *ccb;
% + int error;
% +
% + error = 0;
% + ccb = cdgetccb(periph, /* priority */ 1);
% + csio = &ccb->csio;
% +
% + cam_fill_csio(csio,
% + /* retries */ 1,
% + /* cbfcnp */ cddone,
% + /* flags */ CAM_DIR_NONE,
% + /* tag_action */ MSG_SIMPLE_Q_TAG,
% + /* data_ptr */ NULL,
% + /* dxfer_len */ 0,
% + /* sense_len */ SSD_FULL_SIZE,
% + sizeof(struct scsi_set_speed),
% + /* timeout */ 50000);
% +
% + scsi_cmd = (struct scsi_set_speed *)&csio->cdb_io.cdb_bytes;
% + bzero(scsi_cmd, sizeof(*scsi_cmd));
% +
% + scsi_cmd->opcode = SET_CD_SPEED;
% + scsi_ulto2b(rdspeed, scsi_cmd->readspeed);
% + scsi_ulto2b(wrspeed, scsi_cmd->writespeed);
%
% error = cdrunccb(ccb, cderror, /*cam_flags*/CAM_RETRY_SELTO,
Part of the recent patch for speeds.
Bruce
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?20021019152522.T1864-100000>