SCSI not showing tray status correctly
Quoted 10 lines
This patch is too simplistic. ide-cd.c:ide_cdrom_drive_status() looks to be a reasonable implementation. However, the worry is that
GET_EVENT_NOTIFICATIONis a MMC command; devices not conforming to MMC won't support it. In theory, they should just returnILLEGAL_REQUEST, but USB devices have been known to crash when given commands they don't understand. How widely tested has this been (if it's been in Gentoo since 2004, then it's probably widely tested enough)?James
This patch hasn't been tested at all, I'm sorry if I gave you that impression, it isn't in Gentoo's patches, it's just been brought to our attention in the bug I linked too.
I created another patch, based on your recommendations, and with a lot of help from Daniel Drake ([email protected]), that's included below. Some changes are made to test_unit_ready() to be able to pass the sense data to sr_drive_status(). Currently, sr_do_ioctl() swallows this data and makes its own interpretations of sense codes, which isn't what we want here.
Is this what you had in mind? Do you think the possible problems with USB drives that you mentioned will prevent this patch from going in? The patch has only been compile tested right now, we can do some real testing later, for now I'd just like to get your feedback on it.
Maarten
---
drivers/scsi/sr_ioctl.c +42 -10
@@ -275,16 +275,21 @@ int sr_do_ioctl(Scsi_CD *cd, struct pack/* ---------------------------------------------------------------------- */ /* interface to cdrom.c */ -static int test_unit_ready (Scsi_CD *cd) +static int test_unit_ready (Scsi_CD *cd,struct request_sense *sense) { -struct packet_command cgc; +struct scsi_device *SDev = cd->device; +unsigned char cmd[CDROM_PACKET_SIZE ] = {GPCMD_TEST_UNIT_READY }; +int result; -memset (&cgc,0 ,sizeof (struct packet_command )); - cgc.cmd[0 ] =GPCMD_TEST_UNIT_READY ; - cgc.quiet =1 ; - cgc.data_direction =DMA_NONE ; - cgc.timeout =IOCTL_TIMEOUT ; -return sr_do_ioctl (cd, &cgc); +if (!scsi_block_when_processing_errors (SDev)) +return -ENODEV ; + +memset (sense,0 ,sizeof (*sense)); + result =scsi_execute (SDev, cmd,DMA_NONE , +NULL ,0 , (char *)sense, +IOCTL_TIMEOUT ,IOCTL_RETRIES ,0 ); + +return driver_byte (result); }int sr_tray_move (struct cdrom_device_info *cdi,int pos)
@@ -310,14 +315,41 @@ int sr_lock_door(struct cdrom_device_infint sr_drive_status (struct cdrom_device_info *cdi,int slot) { +struct request_sense sense; +struct media_event_desc med; +if (CDSL_CURRENT != slot) {/* we have no changer support */ return -EINVAL ; } -if (0 ==test_unit_ready (cdi->handle)) +if ((0 ==test_unit_ready (cdi->handle, &sense)) || + sense.sense_key ==UNIT_ATTENTION )return CDS_DISC_OK ; -return CDS_TRAY_OPEN ; +if (!cdrom_get_media_event (cdi, &med)) { +if (med.media_present) +return CDS_DISC_OK ; +else if (med.door_open) +return CDS_TRAY_OPEN ; +else +return CDS_NO_DISC ; + } + +if (sense.sense_key ==NOT_READY && sense.asc ==0x04 && sense.ascq ==0x04 ) +return CDS_DISC_OK ; + +/* +* If not using Mt Fuji extended media tray reports, +* just return TRAY_OPEN since ATAPI doesn't provide +* any other way to detect this... +*/ +if (sense.sense_key ==NOT_READY ) { +if (sense.asc ==0x3a && sense.ascq ==1 ) +return CDS_NO_DISC ; +else +return CDS_TRAY_OPEN ; + } +return CDS_DRIVE_NOT_READY ; }int sr_disk_status (struct cdrom_device_info *cdi)
Quoted 21 lines · Maarten Bressers
Quoted 10 lines
This patch is too simplistic. ide-cd.c:ide_cdrom_drive_status() looks to be a reasonable implementation. However, the worry is that
GET_EVENT_NOTIFICATIONis a MMC command; devices not conforming to MMC won't support it. In theory, they should just returnILLEGAL_REQUEST, but USB devices have been known to crash when given commands they don't understand. How widely tested has this been (if it's been in Gentoo since 2004, then it's probably widely tested enough)?James
This patch hasn't been tested at all, I'm sorry if I gave you that impression, it isn't in Gentoo's patches, it's just been brought to our attention in the bug I linked too.
I created another patch, based on your recommendations, and with a lot of help from Daniel Drake ([email protected]), that's included below. Some changes are made to
test_unit_ready()to be able to pass the sense data tosr_drive_status(). Currently,sr_do_ioctl()swallows this data and makes its own interpretations of sense codes, which isn't what we want here.
Quoted 4 lines
Is this what you had in mind? Do you think the possible problems with USB drives that you mentioned will prevent this patch from going in? The patch has only been compile tested right now, we can do some real testing later, for now I'd just like to get your feedback on it.
In general it looks good ... I suppose the only way to test it is to stick it in and see what happens. However, there are a few rough edges.
Quoted 23 lines
Maarten
---
drivers/scsi/sr_ioctl.c +4
-7@@ -275,16 +275,21 @@ int sr_do_ioctl(Scsi_CD *cd, struct pack/* ---------------------------------------------------------------------- */ /* interface to cdrom.c */ -static int test_unit_ready (Scsi_CD *cd) +static int test_unit_ready (Scsi_CD *cd,struct request_sense *sense) { -struct packet_command cgc; +struct scsi_device *SDev = cd->device; +unsigned char cmd[CDROM_PACKET_SIZE ] = {GPCMD_TEST_UNIT_READY }; +int result; -memset (&cgc,0 ,sizeof (struct packet_command )); - cgc.cmd[0 ] =GPCMD_TEST_UNIT_READY ; - cgc.quiet =1 ; - cgc.data_direction =DMA_NONE ; - cgc.timeout =IOCTL_TIMEOUT ;
drivers/scsi/sr_ioctl.c int sr_do_ioctl(Scsi_CD *cd, struct pack · line 292
Quoted 10 lines
drivers/scsi/sr_ioctl.c +9
-1-return sr_do_ioctl (cd, &cgc); +if (!scsi_block_when_processing_errors (SDev)) +return -ENODEV ; + +memset (sense,0 ,sizeof (*sense)); + result =scsi_execute (SDev, cmd,DMA_NONE , +NULL ,0 , (char *)sense, +IOCTL_TIMEOUT ,IOCTL_RETRIES ,0 ); + +return driver_byte (result);
This bit isn't quite right ... this will return true if there's a collected sense code and false otherwise (so it returns 0 on failure that doesn't produce any sense information).
Quoted 7 lines
drivers/scsi/sr_ioctl.c +0
-0}int sr_tray_move (struct cdrom_device_info *cdi,int pos)@@ -310,14 +315,41 @@ int sr_lock_door(struct cdrom_device_infint sr_drive_status (struct cdrom_device_info *cdi,int slot) {
drivers/scsi/sr_ioctl.c int sr_lock_door(struct cdrom_device_inf · line 326
Quoted 10 lines
drivers/scsi/sr_ioctl.c +5
-1+struct request_sense sense; +struct media_event_desc med; +if (CDSL_CURRENT != slot) {/* we have no changer support */ return -EINVAL ; } -if (0 ==test_unit_ready (cdi->handle)) +if ((0 ==test_unit_ready (cdi->handle, &sense)) || + sense.sense_key ==UNIT_ATTENTION )
Unit attention won't necessarily mean the media is OK ... unit attention can mean the media or tray status has changed.
drivers/scsi/sr_ioctl.c int sr_lock_door(struct cdrom_device_inf · line 338
Quoted 13 lines
drivers/scsi/sr_ioctl.c +10
-1return CDS_DISC_OK ; -return CDS_TRAY_OPEN ; +if (!cdrom_get_media_event (cdi, &med)) { +if (med.media_present) +return CDS_DISC_OK ; +else if (med.door_open) +return CDS_TRAY_OPEN ; +else +return CDS_NO_DISC ; + } + +if (sense.sense_key ==NOT_READY && sense.asc ==0x04 && sense.ascq ==0x04 )
Actually, all asc 0x4 codes are some type of in progress operation, format doesn't necessarily have to be treated specially.
drivers/scsi/sr_ioctl.c int sr_lock_door(struct cdrom_device_inf · line 350
Quoted 12 lines
drivers/scsi/sr_ioctl.c +12
-0+return CDS_DISC_OK ; + +/* +* If not using Mt Fuji extended media tray reports, +* just return TRAY_OPEN since ATAPI doesn't provide +* any other way to detect this... +*/ +if (sense.sense_key ==NOT_READY ) { +if (sense.asc ==0x3a && sense.ascq ==1 ) +return CDS_NO_DISC ; +else +return CDS_TRAY_OPEN ;
This is also a bit odd. asc 0x3a ascq 0x2 definitely means tray open, but there are a lot of other not ready conditions that don't mean this.
drivers/scsi/sr_ioctl.c int sr_lock_door(struct cdrom_device_inf · line 355
Quoted 5 lines
drivers/scsi/sr_ioctl.c +2
-0+ } +return CDS_DRIVE_NOT_READY ; }int sr_disk_status (struct cdrom_device_info *cdi)
James
James Bottomley
maintainerPATCH 1/1 sr: update to follow tray status correctly
It's been a while with no status on this one, so I corrected the patch based on my original input. The attached is what I think is the best way of doing this (I've replaced the home grown test_unit_ready routine with the SCSI one and updated some of the sense check conditions).
It seems to work for me.
James
---
drivers/scsi/sr.c +0 -2
@@ -67,8 +67,6 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM);#define SR_DISKS 256 -#define MAX_RETRIES 3 -#define SR_TIMEOUT (30 * HZ)#define SR_CAPABILITIES \ (CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_SPEED| \ CDC_SELECT_DISC|CDC_MULTI_SESSION|CDC_MCN|CDC_MEDIA_CHANGED| \
drivers/scsi/sr.h +3 -0
@@ -20,6 +20,9 @@#include <linux/genhd.h> #include <linux/kref.h> +#define MAX_RETRIES 3 +#define SR_TIMEOUT (30 * HZ) +struct scsi_device ;/* The CDROM is fairly slow, so we need a little extra time */
drivers/scsi/sr_ioctl.c +34 -14
66 lines not on this page · raw message
Re: [PATCH] sr: update to follow tray status correctly
Hi James,
Quoted 4 lines · James Bottomley
It's been a while with no status on this one, so I corrected the patch based on my original input. The attached is what I think is the best way of doing this (I've replaced the home grown
test_unit_readyroutine with the SCSI one and updated some of the sense check conditions).
Many thanks for following up on this. The user has not responded to my request for him to test your patch, so I decided to try it myself. I'm using Linus git as of today, I noticed the patch is merged.
It doesn't work for me. On unpatched 2.6.24, I get the behaviour previously described - the ioctl always gives CDS_TRAY_OPEN regardless of tray status. On 2.6.24-git which includes your patch, I instead always get CDS_DISC_OK. I have confirmed that on 2 systems this is because scsi_test_unit_ready() is returning 0.
Before I dig further, does this work for you? I have attached the test program I am using.
Thanks!
Daniel
#include <sys/ioctl.h>
#include <sys/types.h>
#include <fcntl.h>
#include <linux/cdrom.h>
#include <unistd.h>
#include <stdio.h>
int main(void)
{
int fd = open("/dev/sr0", O_RDONLY | O_NONBLOCK);
int ret;
if (fd < 0) {
perror("open");
return 1;
}
ret = ioctl(fd, CDROM_DRIVE_STATUS, 0);
printf("ioctl result %d\n", ret);
switch(ret) {
case CDS_NO_INFO: printf("CDS_NO_INFO\n"); break;
case CDS_NO_DISC: printf("CDS_NO_DISC\n"); break;
case CDS_TRAY_OPEN: printf("CDS_TRAY_OPEN\n"); break;
case CDS_DRIVE_NOT_READY: printf("CDS_DRIVE_NOT_READY\n"); break;
case CDS_DISC_OK: printf("CDS_DISC_OK\n"); break;
default: printf("Unknown\n"); break;
}
return 0;
}James Bottomley
maintainerRe: [PATCH] sr: update to follow tray status correctly
Quoted 12 lines · Daniel Drake
Hi James,
Quoted 4 lines · James Bottomley
It's been a while with no status on this one, so I corrected the patch based on my original input. The attached is what I think is the best way of doing this (I've replaced the home grown
test_unit_readyroutine with the SCSI one and updated some of the sense check conditions).Many thanks for following up on this. The user has not responded to my request for him to test your patch, so I decided to try it myself. I'm using Linus git as of today, I noticed the patch is merged.
Quoted 8 lines
It doesn't work for me. On unpatched 2.6.24, I get the behaviour previously described - the ioctl always gives
CDS_TRAY_OPENregardless of tray status. On 2.6.24-git which includes your patch, I instead always getCDS_DISC_OK. I have confirmed that on 2 systems this is becausescsi_test_unit_ready()is returning 0.Before I dig further, does this work for you? I have attached the test program I am using.
Heh, you assume I have a CD easily to hand in my testing setup. But, unfortunately it requires a bit of manual intervention to set up the state ...
However, I think you're right, the vanilla TUR does eat NOT_READY for removable media, which CDs are. Does this fix it?
James
---
drivers/scsi/sr.c +27 -22
@@ -163,6 +163,29 @@ static void scsi_cd_put(struct scsi_cd *cd)mutex_unlock (&sr_ref_mutex); } +/* identical to scsi_test_unit_ready except that it doesn't +* eat the NOT_READY returns for removable media */ +int sr_test_unit_ready (struct scsi_device *sdev,struct scsi_sense_hdr *sshdr) +{ +int retries =MAX_RETRIES ; +int the_result; +u8 cmd[] = {TEST_UNIT_READY ,0 ,0 ,0 ,0 ,0 }; + +/* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION +* conditions are gone, or a timeout happens +*/ +do { + the_result =scsi_execute_req (sdev, cmd,DMA_NONE ,NULL , +0 , sshdr,SR_TIMEOUT , + retries--); + + }while (retries >0 && + (!scsi_status_is_good (the_result) || + (scsi_sense_valid (sshdr) && + sshdr->sense_key ==UNIT_ATTENTION ))); +return the_result; +} +/* * This function checks to see if the media has been changed in the * CDROM drive. It is possible that we have already sensed a change,
@@ -185,8 +208,7 @@ static int sr_media_change(struct cdrom_device_info *cdi, int slot) } sshdr =kzalloc (sizeof (*sshdr),GFP_KERNEL ); - retval =scsi_test_unit_ready (cd->device,SR_TIMEOUT ,MAX_RETRIES , - sshdr); + retval =sr_test_unit_ready (cd->device, sshdr);if (retval || (scsi_sense_valid (sshdr) &&/* 0x3a is medium not present */ sshdr->asc ==0x3a )) {
@@ -733,10 +755,8 @@ static void get_capabilities(struct scsi_cd *cd) {unsigned char *buffer;struct scsi_mode_data data; -unsigned char cmd[MAX_COMMAND_SIZE ];struct scsi_sense_hdr sshdr; -unsigned int the_result; -int retries, rc, n; +int rc, n;static const char *loadmech[] = {
@@ -758,23 +778,8 @@ static void get_capabilities(struct scsi_cd *cd)return ; } -/* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION -* conditions are gone, or a timeout happens -*/ - retries =0 ; -do { -memset ((void *)cmd,0 ,MAX_COMMAND_SIZE ); - cmd[0 ] =TEST_UNIT_READY ; - - the_result =scsi_execute_req (cd->device, cmd,DMA_NONE ,NULL , -0 , &sshdr,SR_TIMEOUT , -MAX_RETRIES ); - - retries++; - }while (retries <5 && - (!scsi_status_is_good (the_result) || - (scsi_sense_valid (&sshdr) && - sshdr.sense_key ==UNIT_ATTENTION ))); +/* eat unit attentions */ +sr_test_unit_ready (cd->device, &sshdr);/* ask for mode page 0x2a */ rc = scsi_mode_sense(cd->device, 0, 0x2a, buffer, 128,
drivers/scsi/sr.h +1 -0
@@ -61,6 +61,7 @@ int sr_select_speed(struct cdrom_device_info *cdi, int speed);int sr_audio_ioctl (struct cdrom_device_info *,unsigned int ,void *);int sr_is_xa (Scsi_CD *); +int sr_test_unit_ready (struct scsi_device *sdev,struct scsi_sense_hdr *sshdr);/* sr_vendor.c */ void sr_vendor_init (Scsi_CD *);
drivers/scsi/sr_ioctl.c +1 -2
@@ -306,8 +306,7 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)/* we have no changer support */ return -EINVAL ; } -if (0 ==scsi_test_unit_ready (cd->device,SR_TIMEOUT ,MAX_RETRIES , - &sshdr)) +if (0 ==sr_test_unit_ready (cd->device, &sshdr))return CDS_DISC_OK ;if (!cdrom_get_media_event (cdi, &med)) {
Re: [PATCH] sr: update to follow tray status correctly
Quoted 2 lines · James Bottomley
However, I think you're right, the vanilla TUR does eat
NOT_READYfor removable media, which CDs are. Does this fix it?
Works great, thanks!
Daniel