[PATCH] IDE block device trouble, mostly CD-related

From: Tobias Ringström (zajbot@goteborg.utfors.se)
Date: Tue Apr 04 2000 - 16:55:53 EST


Hi!

Ever since I switched from a SCSI CD-ROM drive to an ATAPI drive, I
have not been able to run "dd if=/dev/hdc of=cd.iso" to create an
image of a CD. The symptoms vary between different kernel versions.
Normally you get I/O errors because dd tries to read past the end of
the CD, but on 2.3 kernels you get I/O errors even before the end.
Using the IDE SCSI emulation, "dd if=/dev/scd0 of=cd.iso" works fine.
[I am aware that it is OK for the track to end before the end of the
CD capacity, but it is not very common, at least not for regular
non-mixed data CDs.]

This is what I found (linux-2.3.99-pre3):

1. cdrom_read_capacity() returns one frame too little. This is
   because the ATAPI command returns the number of the last block, not
   the number of blocks.

2. default_capacity() is used for ide-cd, which basically means that
   the capacity of the CD-ROM is not read.

3. ide_cdrom_check_media_change_real() always return 0. This looks so
   strange that I almost think it is intentional. If it is, there should
   be a comment in the code about this.

4. If point 3 above is fixed, grok_partitions() is called for the
   CD-ROM device. This function does nothing if the number of minors
   is one, which is normally the case. However, there is code in
   ide-cd.c to give each session its own minor number. I suspect that
   this will give some strange results, but I have not verified that.

5. The code to give each session its own minor number is very broken. It
   always sets the start sector to zero, and the number of sectors to the
   lba of start of the next track. I have removed this code.

6. blk_size is not used at all. This makes block_read() unaware of
   how the size of the block device. It also tries to read_ahead past
   the end of the device, resulting in ugly log messages.

7. There is strange comment in grok_partitions(): "No Such
   Agen^Wdevice or no minors to use for partitions". Pardon?

I have attached a patch that fix point 1-6. The largest change I made
was to add a revalidate function for all ide types. I moved the
grok_partitions() call to ide-disk and ide-floppy, called
cdrom_read_toc() for ide-cd, and did nothing for ide-tape. I do not
have access to any IDE floppy or tape devices, so I cannot verify that
it works.

One surprising thing is I cannot eject the disc with the eject button
directly after reading the capacity (cat /proc/ide/hdc/capacity).
After I read something from the disc, I can eject it again. I cannot
explain this.

I think that the idea of using "partitions" for tracks/sessions is a
good idea, if it can be done correctly. Probably only data tracks
should be made visible. It would be really nice to find out the true
sizes of the tracks (e.g. excluding the post-gap crap), to avoid read
errors at the end of the track. I tried to use the
GPCMD_READ_TRACK_RZONE_INFO command to do this, but it did not return
reliable values. Any ideas of how to do this?

I look forward to your comments.

/Tobias

diff -u --recursive --new-file linux-2.3.99-pre3/drivers/ide/ide-cd.c linux-2.3.99-pre3-new/drivers/ide/ide-cd.c
--- linux-2.3.99-pre3/drivers/ide/ide-cd.c Mon Apr 3 00:37:15 2000
+++ linux-2.3.99-pre3-new/drivers/ide/ide-cd.c Tue Apr 4 23:43:35 2000
@@ -1584,7 +1584,7 @@
 
         stat = cdrom_queue_packet_command(drive, &pc);
         if (stat == 0)
- *capacity = be32_to_cpu(capbuf.lba);
+ *capacity = 1 + be32_to_cpu(capbuf.lba);
 
         return stat;
 }
@@ -1617,7 +1617,6 @@
         int stat, ntracks, i;
         struct cdrom_info *info = drive->driver_data;
         struct atapi_toc *toc = info->toc;
- int minor = drive->select.b.unit << PARTN_BITS;
         struct {
                 struct atapi_toc_header hdr;
                 struct atapi_toc_entry ent;
@@ -1752,32 +1751,9 @@
         stat = cdrom_read_capacity (drive, &toc->capacity);
         if (stat) toc->capacity = 0x1fffff;
 
- /* for general /dev/cdrom like mounting, one big disc */
- drive->part[0].nr_sects = toc->capacity * SECTORS_PER_FRAME;
- HWIF(drive)->gd->sizes[minor] = (toc->capacity * SECTORS_PER_FRAME) >>
- (BLOCK_SIZE_BITS - 9);
-
         /* Remember that we've read this stuff. */
         CDROM_STATE_FLAGS (drive)->toc_valid = 1;
 
- /* should be "if multisession", but it does no harm. */
- if (ntracks == 1)
- return 0;
-
- /* setup each minor to respond to a session */
- minor++;
- i = toc->hdr.first_track;
- while ((i <= ntracks) && ((minor & CD_PART_MASK) < CD_PART_MAX)) {
- drive->part[minor & PARTN_MASK].start_sect = 0;
- drive->part[minor & PARTN_MASK].nr_sects =
- (toc->ent[i].addr.lba *
- SECTORS_PER_FRAME) << (BLOCK_SIZE_BITS - 9);
- HWIF(drive)->gd->sizes[minor] = (toc->ent[i].addr.lba *
- SECTORS_PER_FRAME) >> (BLOCK_SIZE_BITS - 9);
- i++;
- minor++;
- }
-
         return 0;
 }
 
@@ -2183,11 +2159,13 @@
                                        int slot_nr)
 {
         ide_drive_t *drive = (ide_drive_t*) cdi->handle;
+ int retval;
         
         if (slot_nr == CDSL_CURRENT) {
                 (void) cdrom_check_status(drive);
+ retval = CDROM_STATE_FLAGS (drive)->media_changed;
                 CDROM_STATE_FLAGS (drive)->media_changed = 0;
- return CDROM_STATE_FLAGS (drive)->media_changed;
+ return retval;
         } else {
                 return -EINVAL;
         }
@@ -2601,6 +2579,34 @@
 }
 
 static
+void ide_cdrom_revalidate (ide_drive_t *drive)
+{
+ struct cdrom_info *info = drive->driver_data;
+ struct atapi_toc *toc = info->toc;
+ int minor = drive->select.b.unit << PARTN_BITS;
+
+ cdrom_read_toc(drive);
+
+ if (!CDROM_STATE_FLAGS (drive)->toc_valid)
+ return;
+
+ /* for general /dev/cdrom like mounting, one big disc */
+ drive->part[0].nr_sects = toc->capacity * SECTORS_PER_FRAME;
+ HWIF(drive)->gd->sizes[minor] = toc->capacity * BLOCKS_PER_FRAME;
+
+ blk_size[HWIF(drive)->major] = HWIF(drive)->gd->sizes;
+}
+
+static
+unsigned long ide_cdrom_capacity (ide_drive_t *drive)
+{
+ unsigned capacity;
+
+ return cdrom_read_capacity(drive, &capacity) ? 0 :
+ capacity * SECTORS_PER_FRAME;
+}
+
+static
 int ide_cdrom_cleanup(ide_drive_t *drive)
 {
         struct cdrom_info *info = drive->driver_data;
@@ -2635,8 +2641,9 @@
         ide_cdrom_open, /* open */
         ide_cdrom_release, /* release */
         ide_cdrom_check_media_change, /* media_change */
+ ide_cdrom_revalidate, /* revalidate */
         NULL, /* pre_reset */
- NULL, /* capacity */
+ ide_cdrom_capacity, /* capacity */
         NULL, /* special */
         NULL /* proc */
 };
diff -u --recursive --new-file linux-2.3.99-pre3/drivers/ide/ide-cd.h linux-2.3.99-pre3-new/drivers/ide/ide-cd.h
--- linux-2.3.99-pre3/drivers/ide/ide-cd.h Sat Mar 25 12:28:39 2000
+++ linux-2.3.99-pre3-new/drivers/ide/ide-cd.h Tue Apr 4 19:21:45 2000
@@ -43,6 +43,8 @@
 #define SECTOR_BUFFER_SIZE (CD_FRAMESIZE * 32)
 #define SECTORS_BUFFER (SECTOR_BUFFER_SIZE / SECTOR_SIZE)
 
+#define BLOCKS_PER_FRAME (CD_FRAMESIZE / BLOCK_SIZE)
+
 #define MIN(a,b) ((a) < (b) ? (a) : (b))
 
 /* special command codes for strategy routine. */
diff -u --recursive --new-file linux-2.3.99-pre3/drivers/ide/ide-disk.c linux-2.3.99-pre3-new/drivers/ide/ide-disk.c
--- linux-2.3.99-pre3/drivers/ide/ide-disk.c Sat Mar 25 12:22:15 2000
+++ linux-2.3.99-pre3-new/drivers/ide/ide-disk.c Tue Apr 4 23:45:31 2000
@@ -512,6 +512,13 @@
         return drive->removable; /* if removable, always assume it was changed */
 }
 
+static void idedisk_revalidate (ide_drive_t *drive)
+{
+ grok_partitions(HWIF(drive)->gd, drive->select.b.unit,
+ 1<<PARTN_BITS,
+ current_capacity(drive));
+}
+
 /*
  * Compute drive->capacity, the full capacity of the drive
  * Called with drive->id != NULL.
@@ -726,6 +733,7 @@
         idedisk_open, /* open */
         idedisk_release, /* release */
         idedisk_media_change, /* media_change */
+ idedisk_revalidate, /* revalidate */
         idedisk_pre_reset, /* pre_reset */
         idedisk_capacity, /* capacity */
         idedisk_special, /* special */
diff -u --recursive --new-file linux-2.3.99-pre3/drivers/ide/ide-floppy.c linux-2.3.99-pre3-new/drivers/ide/ide-floppy.c
--- linux-2.3.99-pre3/drivers/ide/ide-floppy.c Sat Mar 25 12:22:15 2000
+++ linux-2.3.99-pre3-new/drivers/ide/ide-floppy.c Tue Apr 4 23:45:46 2000
@@ -1376,6 +1376,16 @@
 }
 
 /*
+ * Revalidate the new media. Should set blk_size[]
+ */
+static void idefloppy_revalidate (ide_drive_t *drive)
+{
+ grok_partitions(HWIF(drive)->gd, drive->select.b.unit,
+ 1<<PARTN_BITS,
+ current_capacity(drive));
+}
+
+/*
  * Return the current floppy capacity to ide.c.
  */
 static unsigned long idefloppy_capacity (ide_drive_t *drive)
@@ -1604,6 +1614,7 @@
         idefloppy_open, /* open */
         idefloppy_release, /* release */
         idefloppy_media_change, /* media_change */
+ idefloppy_revalidate, /* media_change */
         NULL, /* pre_reset */
         idefloppy_capacity, /* capacity */
         NULL, /* special */
diff -u --recursive --new-file linux-2.3.99-pre3/drivers/ide/ide-tape.c linux-2.3.99-pre3-new/drivers/ide/ide-tape.c
--- linux-2.3.99-pre3/drivers/ide/ide-tape.c Sat Mar 25 12:22:15 2000
+++ linux-2.3.99-pre3-new/drivers/ide/ide-tape.c Mon Apr 3 00:21:44 2000
@@ -5900,6 +5900,7 @@
         idetape_blkdev_open, /* open */
         idetape_blkdev_release, /* release */
         NULL, /* media_change */
+ NULL, /* revalidate */
         idetape_pre_reset, /* pre_reset */
         NULL, /* capacity */
         NULL, /* special */
diff -u --recursive --new-file linux-2.3.99-pre3/drivers/ide/ide.c linux-2.3.99-pre3-new/drivers/ide/ide.c
--- linux-2.3.99-pre3/drivers/ide/ide.c Sat Mar 25 12:22:15 2000
+++ linux-2.3.99-pre3-new/drivers/ide/ide.c Tue Apr 4 12:58:27 2000
@@ -1776,10 +1776,7 @@
                 drive->part[p].nr_sects = 0;
         };
 
- grok_partitions(HWIF(drive)->gd, drive->select.b.unit,
- (drive->media != ide_disk &&
- drive->media != ide_floppy) ? 1 : 1<<PARTN_BITS,
- current_capacity(drive));
+ DRIVER(drive)->revalidate(drive);
 
         drive->busy = 0;
         wake_up(&drive->wqueue);
@@ -3349,7 +3346,7 @@
 
 static unsigned long default_capacity (ide_drive_t *drive)
 {
- return 0x7fffffff; /* cdrom or tape */
+ return 0x7fffffff;
 }
 
 static ide_startstop_t default_special (ide_drive_t *drive)
diff -u --recursive --new-file linux-2.3.99-pre3/drivers/scsi/ide-scsi.c linux-2.3.99-pre3-new/drivers/scsi/ide-scsi.c
--- linux-2.3.99-pre3/drivers/scsi/ide-scsi.c Fri Jan 21 22:29:11 2000
+++ linux-2.3.99-pre3-new/drivers/scsi/ide-scsi.c Mon Apr 3 00:20:22 2000
@@ -547,6 +547,7 @@
         idescsi_open, /* open */
         idescsi_ide_release, /* release */
         NULL, /* media_change */
+ NULL, /* revalidate */
         NULL, /* pre_reset */
         NULL, /* capacity */
         NULL, /* special */
diff -u --recursive --new-file linux-2.3.99-pre3/include/linux/ide.h linux-2.3.99-pre3-new/include/linux/ide.h
--- linux-2.3.99-pre3/include/linux/ide.h Sat Mar 25 12:28:24 2000
+++ linux-2.3.99-pre3-new/include/linux/ide.h Tue Apr 4 12:15:31 2000
@@ -538,6 +538,7 @@
 typedef int (ide_open_proc)(struct inode *, struct file *, ide_drive_t *);
 typedef void (ide_release_proc)(struct inode *, struct file *, ide_drive_t *);
 typedef int (ide_check_media_change_proc)(ide_drive_t *);
+typedef void (ide_revalidate_proc)(ide_drive_t *);
 typedef void (ide_pre_reset_proc)(ide_drive_t *);
 typedef unsigned long (ide_capacity_proc)(ide_drive_t *);
 typedef ide_startstop_t (ide_special_proc)(ide_drive_t *);
@@ -557,6 +558,7 @@
         ide_open_proc *open;
         ide_release_proc *release;
         ide_check_media_change_proc *media_change;
+ ide_revalidate_proc *revalidate;
         ide_pre_reset_proc *pre_reset;
         ide_capacity_proc *capacity;
         ide_special_proc *special;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Fri Apr 07 2000 - 21:00:13 EST