From owner-freebsd-scsi Fri Oct 18 23:27:36 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 D6D3237B401; Fri, 18 Oct 2002 23:27:20 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1EF4F43E8A; Fri, 18 Oct 2002 23:27:19 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id QAA27729; Sat, 19 Oct 2002 16:26:52 +1000 Date: Sat, 19 Oct 2002 16:37:39 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: "Kenneth D. Merry" Cc: "M. Warner Losh" , , , , Subject: Re: kern/15608: acd0 / cd0 give inconsistent errors on empty tray open() In-Reply-To: <20021013234758.A75169@panzer.kdm.org> Message-ID: <20021019152522.T1864-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 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 #include +#include +#include #include #include @@ -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 % #include % +#include % +#include Needed now since has been depolluted. % #include % #include % +#include Part of the recent patch for speeds. Not related to axing. % #include % #include % @@ -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