make sr_mod report more accurate drive status after closing the tray.
PATCH 1/1 make sr_mod report more accurate drive status after closing the tray.
Right now, when using sr_mod and issuing the CDROM_DRIVE_STATUS ioctl, there's no way to differentiate between when you've just closed the drive tray, but the media is not yet loaded, and when there's no media. This seems to be accidental.
Here's a patch that seems to fix this behaviour:
drivers/scsi/sr_ioctl.c +23 -7
@@ -306,10 +306,9 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)/* we have no changer support */ return -EINVAL ; } -if (0 ==sr_test_unit_ready (cd->device, &sshdr)) -return CDS_DISC_OK ; - -if (!cdrom_get_media_event (cdi, &med)) { +if (0 ==sr_test_unit_ready (cd->device, &sshdr) + && sshdr.sense_key ==0 + && !cdrom_get_media_event (cdi, &med)) {if (med.media_present)return CDS_DISC_OK ;else if (med.door_open)
@@ -319,10 +318,27 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot) }/* -* 0x04 is format in progress .. but there must be a disc present! +* ASC: 0x04: "logical unit is not ready" +* ASCQ: 0x01: cause not reportable +* 0x02: in process of becoming ready +* 0x03: initializing command required +* 0x04: format in progress .. but there must be a disc present! +* 0x07: operation in progress +* 0x08: long write in progress */ -if (sshdr.sense_key ==NOT_READY && sshdr.asc ==0x04 ) -return CDS_DISC_OK ; +if (sshdr.sense_key ==NOT_READY && sshdr.asc ==0x04 ) { +switch (sshdr.ascq) { +case 0x01 : +case 0x02 : +return CDS_DRIVE_NOT_READY ; +case 0x03 : +return CDS_TRAY_OPEN ; +case 0x04 : +case 0x07 : +case 0x08 : +return CDS_DRIVE_NOT_READY ; + } + }/* * If not using Mt Fuji extended media tray reports,
James Bottomley
maintainerQuoted 6 lines · Peter Jones
Right now, when using
sr_modand issuing theCDROM_DRIVE_STATUSioctl, there's no way to differentiate between when you've just closed the drive tray, but the media is not yet loaded, and when there's no media. This seems to be accidental.Here's a patch that seems to fix this behaviour:
I'm very wary of doing something like this because it took us months to fix up all the breakage the last time ...
Quoted 7 lines
Signed-off-by Peter Jones <[email protected]>drivers/scsi/sr_ioctl.c +0
-0
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, · line 311
Quoted 10 lines
drivers/scsi/sr_ioctl.c +2
-4@@ -306,10 +306,9 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)/* we have no changer support */ return -EINVAL ; } -if (0 ==sr_test_unit_ready (cd->device, &sshdr)) -return CDS_DISC_OK ; - -if (!cdrom_get_media_event (cdi, &med)) { +if (0 ==sr_test_unit_ready (cd->device, &sshdr) + && sshdr.sense_key == 0
This can't be right; if the return is zero, the sense_key must also be zero (as in to have valid sense, it must have returned with at least DRIVER_SENSE)
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, int slot) · line 311
Quoted 1 line
drivers/scsi/sr_ioctl.c +1
-0+ && !cdrom_get_media_event(cdi, &med)) {
And this really doesn't look right either. Now you're only calling the media event if the test unit ready succeeded. A drive can be open (thus returning not ready and hence non zero) and still give you a valid media event.
Quoted 9 lines
drivers/scsi/sr_ioctl.c +0
-0if (med.media_present)return CDS_DISC_OK ;else if (med.door_open)
drivers/scsi/sr_ioctl.c +0
-1@@ -319,10 +318,27 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot) }/* -* 0x04 is format in progress .. but there must be a disc present!
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, · line 326
Quoted 10 lines
drivers/scsi/sr_ioctl.c +7
-2+* ASC: 0x04: "logical unit is not ready" +* ASCQ: 0x01: cause not reportable +* 0x02: in process of becoming ready +* 0x03: initializing command required +* 0x04: format in progress .. but there must be a disc present! +* 0x07: operation in progress +* 0x08: long write in progress */ -if (sshdr.sense_key ==NOT_READY && sshdr.asc ==0x04 ) -return CDS_DISC_OK ;
This seems to be a nasty historical lie. It seems to play into the new media changed stuff by stalling the change until the media is ready. Convince me that if we actually tell the truth here we're not going to fire a slew of spurious events at hal.
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, int slot) · line 335
Quoted 7 lines
drivers/scsi/sr_ioctl.c +7
-0+if (sshdr.sense_key ==NOT_READY && sshdr.asc ==0x04 ) { +switch (sshdr.ascq) { +case 0x01 : +case 0x02 : +return CDS_DRIVE_NOT_READY ; +case 0x03 : +return CDS_TRAY_OPEN ;
That's a stretch ... the initialising command can be start motor, but the tray can be closed just fine.
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, int slot) · line 341
Quoted 6 lines
drivers/scsi/sr_ioctl.c +6
-0+case 0x04 : +case 0x07 : +case 0x08 : +return CDS_DRIVE_NOT_READY ; + } + }
James
A message that is not in the archive (sent off-list or not yet received): 1215726719.3353.92.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Quoted 10 lines · James Bottomley
Quoted 6 lines · Peter Jones
Right now, when using
sr_modand issuing theCDROM_DRIVE_STATUSioctl, there's no way to differentiate between when you've just closed the drive tray, but the media is not yet loaded, and when there's no media. This seems to be accidental.Here's a patch that seems to fix this behaviour:
I'm very wary of doing something like this because it took us months to fix up all the breakage the last time ...
That's certainly understandable. What I'm hitting is that I can't tell the difference any more between not having media and it simply not being ready yet. And the amount of time it takes to become ready seems to vary wildly (1s on one drive I've got here, ~20s on another fairly new drive), so a timeout isn't even a very effective kludge.
Quoted 22 lines
Quoted 7 lines
Signed-off-by Peter Jones <[email protected]>drivers/scsi/sr_ioctl.c +0
-0drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, · line 311
Quoted 10 lines
drivers/scsi/sr_ioctl.c +2
-4@@ -306,10 +306,9 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)/* we have no changer support */ return -EINVAL ; } -if (0 ==sr_test_unit_ready (cd->device, &sshdr)) -return CDS_DISC_OK ; - -if (!cdrom_get_media_event (cdi, &med)) { +if (0 ==sr_test_unit_ready (cd->device, &sshdr) + && sshdr.sense_key == 0This can't be right; if the return is zero, the
sense_keymust also be zero (as in to have valid sense, it must have returned with at leastDRIVER_SENSE)
Quoted 6 lines
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, int slot) · line 311
Quoted 1 line
drivers/scsi/sr_ioctl.c +1
-0+ && !cdrom_get_media_event(cdi, &med)) {And this really doesn't look right either. Now you're only calling the media event if the test unit ready succeeded. A drive can be open (thus returning not ready and hence non zero) and still give you a valid media event.
Ok, that's a fair point. That part I'm really trying to avoid here is returning the CDS_NO_DSIC case when the sense key says we don't yet know (NOT_READY). So probably that case simply shouldn't be returned until after we do the SK/ASC/ASCQ check.
Quoted 17 lines
Quoted 17 lines
drivers/scsi/sr_ioctl.c +0
-0if (med.media_present)return CDS_DISC_OK ;else if (med.door_open)
drivers/scsi/sr_ioctl.c +7
-1@@ -319,10 +318,27 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot) }/* -* 0x04 is format in progress .. but there must be a disc present! +* ASC: 0x04: "logical unit is not ready" +* ASCQ: 0x01: cause not reportable +* 0x02: in process of becoming ready +* 0x03: initializing command required +* 0x04: format in progress .. but there must be a disc present! +* 0x07: operation in progress +* 0x08: long write in progress
Quoted 8 lines
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, int slot) · line 325
Quoted 3 lines
drivers/scsi/sr_ioctl.c +0
-2*/ -if (sshdr.sense_key ==NOT_READY && sshdr.asc ==0x04 ) -return CDS_DISC_OK ;This seems to be a nasty historical lie. It seems to play into the new media changed stuff by stalling the change until the media is ready. Convince me that if we actually tell the truth here we're not going to fire a slew of spurious events at hal.
Would you be ok with a more minimal approach, where I only change the behaviour of NOT_READY/4/4 ? I understand if you still want more study of the effects WRT udev/hal.
Quoted 10 lines
drivers/scsi/sr_ioctl.c int sr_drive_status(struct cdrom_device_info *cdi, int slot) · line 335
Quoted 7 lines
drivers/scsi/sr_ioctl.c +7
-0+if (sshdr.sense_key ==NOT_READY && sshdr.asc ==0x04 ) { +switch (sshdr.ascq) { +case 0x01 : +case 0x02 : +return CDS_DRIVE_NOT_READY ; +case 0x03 : +return CDS_TRAY_OPEN ;That's a stretch ... the initialising command can be start motor, but the tray can be closed just fine.
Yes, it definitely is. I don't feel strongly about this particular case, but the rationale was that as long as we're not handling that in the driver, then this case essentially means user intervention is required (though there's really no way for the user to know what that would be). CDS_TRAY_OPEN is, AFAICT, the only return value that means that in other cases (i.e., on slot-loaders). This part isn't critical to the functionality I need; it just seemed wrong when I was reading it.
Signature · 1 line
Peter
A message that is not in the archive (sent off-list or not yet received): [email protected]
So, what's happening here is that the drive is reporting a sense of 2/4/1 ("logical unit is becoming ready") from sr_test_unit_ready(), and then we ask for the media event notification before checking that result at all. The check_media_event_descriptor() call isn't getting a check condition, but it's also reporting that the tray is closed and that there's no media. In actuality it doesn't yet know if there's media or not, but there's no way to express that in the media event status field.
My current thought is that if it told us the device isn't yet ready, we should return that immediately, since there's nothing that'll tell us any more data than that reliably:
drivers/scsi/sr_ioctl.c +5 -0
@@ -309,6 +309,11 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)if (0 ==sr_test_unit_ready (cd->device, &sshdr))return CDS_DISC_OK ; +/* SK/ASC/ASCQ of 2/4/1 means "unit is becoming ready" */ +if (scsi_sense_valid (&sshdr) && sshdr.sense_key ==NOT_READY + && sshdr.asc ==0x04 && sshdr.ascq ==0x01 ) +return CDS_DRIVE_NOT_READY ; +if (!cdrom_get_media_event (cdi, &med)) {if (med.media_present)return CDS_DISC_OK ;
Signature · 1 line
Peter