SCSI: early detection of medium not present, updated
PATCH 1/1 SCSI: early detection of medium not present, updated
Guys, I have this marked as needed-in-2.6.24?
From: Alan Stern <[email protected]>
Taken from http://bugzilla.kernel.org/show_bug.cgi?id=8904
An updated (by Albert, I assume) version of the fourteen-month-old patch here:
http://marc.info/?l=linux-kernel&m=115412002912837&w=2
Apparently fixes the bug described at
http://bugzilla.kernel.org/show_bug.cgi?id=8904
Needs some TLC. Perhaps urgently.
---
drivers/scsi/scsi_ioctl.c | 2 +- drivers/scsi/scsi_lib.c | 20 ++++++++++++++++++-- drivers/scsi/sd.c | 2 +- drivers/scsi/sr.c | 15 +++++++++------ include/scsi/scsi_device.h | 2 +- 5 files changed, 30 insertions(+), 11 deletions(-)
drivers/scsi/scsi_ioctl.c renamed from drivers/scsi/scsi_ioctl.c~scsi-early-detection-of-medium-not-present-updated +1 -1
@@ -244,7 +244,7 @@ int scsi_ioctl(struct scsi_device *sdev,return scsi_set_medium_removal (sdev,SCSI_REMOVAL_ALLOW );case SCSI_IOCTL_TEST_UNIT_READY :return scsi_test_unit_ready (sdev,IOCTL_NORMAL_TIMEOUT , -NORMAL_RETRIES ); +NORMAL_RETRIES ,NULL );case SCSI_IOCTL_START_UNIT : scsi_cmd[0 ] =START_STOP ; scsi_cmd[1 ] =0 ;
drivers/scsi/scsi_lib.c renamed from drivers/scsi/scsi_lib.c~scsi-early-detection-of-medium-not-present-updated +18 -2
@@ -2010,15 +2010,26 @@ scsi_mode_sense(struct scsi_device *sdev }EXPORT_SYMBOL (scsi_mode_sense); +/** +* scsi_test_unit_ready - test if unit is ready +* @sdev: scsi device to change the state of. +* @timeout: command timeout +* @retries: number of retries before failing +* @media_maybe_present: 1 if media maybe present or not. +* 0 if media not present. +* +* Returns zero if unsuccessful or an error if TUR failed. +**/ int -scsi_test_unit_ready (struct scsi_device *sdev,int timeout,int retries) +scsi_test_unit_ready (struct scsi_device *sdev,int timeout,int retries,int *media_maybe_present) {char cmd[] = {TEST_UNIT_READY ,0 ,0 ,0 ,0 ,0 , };struct scsi_sense_hdr sshdr;int result; - +int maybe_present =1 ; + result =scsi_execute_req (sdev, cmd,DMA_NONE ,NULL ,0 , &sshdr, timeout, retries);
@@ -2027,10 +2038,15 @@ scsi_test_unit_ready(struct scsi_deviceif ((scsi_sense_valid (&sshdr)) && ((sshdr.sense_key ==UNIT_ATTENTION ) || (sshdr.sense_key ==NOT_READY ))) { +if (sshdr.asc ==0x3A ) + maybe_present =0 ; sdev->changed =1 ; result =0 ; } } + +if (media_maybe_present) + *media_maybe_present = maybe_present;return result; }EXPORT_SYMBOL (scsi_test_unit_ready);
drivers/scsi/sd.c renamed from drivers/scsi/sd.c~scsi-early-detection-of-medium-not-present-updated +1 -1
@@ -767,7 +767,7 @@ static int sd_media_changed(struct gendi retval = -ENODEV ;if (scsi_block_when_processing_errors (sdp)) - retval =scsi_test_unit_ready (sdp,SD_TIMEOUT ,SD_MAX_RETRIES ); + retval =scsi_test_unit_ready (sdp,SD_TIMEOUT ,SD_MAX_RETRIES ,NULL );/* * Unable to test, unit probably not ready. This usually
drivers/scsi/sr.c renamed from drivers/scsi/sr.c~scsi-early-detection-of-medium-not-present-updated +9 -6
@@ -179,18 +179,21 @@ static int sr_media_change(struct cdrom_ {struct scsi_cd *cd = cdi->handle;int retval; +int media_maybe_present;if (CDSL_CURRENT != slot) {/* no changer support */ return -EINVAL ; } - retval =scsi_test_unit_ready (cd->device,SR_TIMEOUT ,MAX_RETRIES ); -if (retval) { -/* Unable to test, unit probably not ready. This usually -* means there is no disc in the drive. Mark as changed, -* and we will figure it out later once the drive is -* available again. */ + retval =scsi_test_unit_ready (cd->device,SR_TIMEOUT ,MAX_RETRIES , + &media_maybe_present); +if (retval || !media_maybe_present) { +/* Media not present or unable to test, unit probably not +* ready. This usually means there is no disc in the drive. +* Mark as changed, and we will figure it out later once +* the drive is available again. +*/ cd->device->changed =1 ;/* This will force a flush, if called from check_disk_change */ retval =1 ;
include/scsi/scsi_device.h renamed from include/scsi/scsi_device.h~scsi-early-detection-of-medium-not-present-updated +1 -1
@@ -292,7 +292,7 @@ extern int scsi_mode_select(struct scsi_struct scsi_mode_data *data, struct scsi_sense_hdr *);extern int scsi_test_unit_ready (struct scsi_device *sdev,int timeout, -int retries); +int retries,int *media_maybe_present);extern int scsi_device_set_state (struct scsi_device *sdev,enum scsi_device_state state); extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
_
Quoted 11 lines · Andrew Morton
From: Alan Stern <[email protected]>
Taken from http://bugzilla.kernel.org/show_bug.cgi?id=8904
An updated (by Albert, I assume) version of the fourteen-month-old patch here:
http://marc.info/?l=linux-kernel&m=115412002912837&w=2
Apparently fixes the bug described at
http://bugzilla.kernel.org/show_bug.cgi?id=8904
Quoted 8 lines
Needs some TLC. Perhaps urgently.
Cc Albert Lee <[email protected]>Cc Alan Stern <[email protected]>Cc James Bottomley <[email protected]>Cc Tejun Heo <[email protected]>Cc Jens Axboe <[email protected]>Signed-off-by Andrew Morton <[email protected]>
I think I did it last time but...
Signature · 1 line
tejun
Quoted 6 lines · Andrew Morton
Guys, I have this marked as needed-in-2.6.24?
From: Alan Stern <[email protected]>
Quoted 8 lines
An updated (by Albert, I assume) version of the fourteen-month-old patch here:
http://marc.info/?l=linux-kernel&m=115412002912837&w=2
Apparently fixes the bug described at
http://bugzilla.kernel.org/show_bug.cgi?id=8904Needs some TLC. Perhaps urgently.
As far as I'm concerned there's no harm in leaving it until 2.6.25; my original version was only a slight performance optimization to prevent excessive querying when the answer is known beforehand. In fact this version of the patch includes only a portion of my original submission. The part in sd.c that exits the retry loop early when there's no medium is missing.
But if this really does fix bug #8904 then it should go in sooner. The question is, does it fix the bug? The report mentions only a small amount of testing.
Alan Stern
Quoted 1 line · Andrew Morton
Guys, I have this marked as needed-in-2.6.24?
Could you wait on this a bit ... since it's such an old bug. The code in question needs to be reworked (as the comment says). I think the best rework is to have the caller pass in an optional struct scsi_sense_header into scsi_test_unit_ready() instead of the hacky media_may_be_present, so the one place that needs this will be able to interpret the sense codes correctly.
I can do this when I get back home ... unfortunately, I'm in Aswan today at an Internet café ... I'll be back in Cairo tomorrow, but my US system seems to have fallen off the internet, so I might not be able to test any patches I come up with. Worst case, I'll be back in Chicago on Thursday.
James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · James Bottomley
Quoted 1 line · Andrew Morton
Guys, I have this marked as needed-in-2.6.24?
Could you wait on this a bit ... since it's such an old bug. The code in question needs to be reworked (as the comment says). I think the best rework is to have the caller pass in an optional struct
scsi_sense_headerintoscsi_test_unit_ready()instead of the hackymedia_may_be_present, so the one place that needs this will be able to interpret the sense codes correctly.
Quoted 5 lines
I can do this when I get back home ... unfortunately, I'm in Aswan today at an Internet café ... I'll be back in Cairo tomorrow, but my US system seems to have fallen off the internet, so I might not be able to test any patches I come up with. Worst case, I'll be back in Chicago on Thursday.
That's fine with me. But the optional scsi_sense_header should be added in two places, not just one: There's the medium-detection code in sr.c already in this patch, and there's the medium-detection code in sd.c (it was changed in my original patch but not in this one).
Alan Stern
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · James Bottomley
Quoted 1 line · Andrew Morton
Guys, I have this marked as needed-in-2.6.24?
Could you wait on this a bit ... since it's such an old bug. The code in question needs to be reworked (as the comment says). I think the best rework is to have the caller pass in an optional struct
scsi_sense_headerintoscsi_test_unit_ready()instead of the hackymedia_may_be_present, so the one place that needs this will be able to interpret the sense codes correctly.
Quoted 5 lines
I can do this when I get back home ... unfortunately, I'm in Aswan today at an Internet café ... I'll be back in Cairo tomorrow, but my US system seems to have fallen off the internet, so I might not be able to test any patches I come up with. Worst case, I'll be back in Chicago on Thursday.
Actually, the night train is a good place to do coding. I know this compiles, but could someone check it out? It's the form I think we should do, since the ASC/ASCQ codes for esoteric conditions which we might eventually check for are different for CDs and removable discs.
James
drivers/scsi/scsi_ioctl.c +1 -1
@@ -244,7 +244,7 @@ int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)return scsi_set_medium_removal (sdev,SCSI_REMOVAL_ALLOW );case SCSI_IOCTL_TEST_UNIT_READY :return scsi_test_unit_ready (sdev,IOCTL_NORMAL_TIMEOUT , -NORMAL_RETRIES ); +NORMAL_RETRIES ,NULL );case SCSI_IOCTL_START_UNIT : scsi_cmd[0 ] =START_STOP ; scsi_cmd[1 ] =0 ;
drivers/scsi/scsi_lib.c +38 -8
65 lines not on this page · raw message
drivers/scsi/sd.c +10 -3
@@ -736,6 +736,7 @@ static int sd_media_changed(struct gendisk *disk) {struct scsi_disk *sdkp =scsi_disk (disk);struct scsi_device *sdp = sdkp->device; +struct scsi_sense_hdr *sshdr =NULL ;int retval;SCSI_LOG_HLQUEUE (3 ,sd_printk (KERN_INFO , sdkp,"sd_media_changed\n" ));
@@ -766,8 +767,11 @@ static int sd_media_changed(struct gendisk *disk)*/ retval = -ENODEV ; -if (scsi_block_when_processing_errors (sdp)) - retval =scsi_test_unit_ready (sdp,SD_TIMEOUT ,SD_MAX_RETRIES ); +if (scsi_block_when_processing_errors (sdp)) { + sshdr =kzalloc (sizeof (*sshdr),GFP_KERNEL ); + retval =scsi_test_unit_ready (sdp,SD_TIMEOUT ,SD_MAX_RETRIES , + sshdr); + }/* * Unable to test, unit probably not ready. This usually
@@ -775,7 +779,9 @@ static int sd_media_changed(struct gendisk *disk)* and we will figure it out later once the drive is * available again. */ -if (retval) { +if (retval || (scsi_sense_valid (sshdr) && +/* 0x3a is medium not present */ + sshdr->asc ==0x3a )) {set_media_not_present (sdkp); retval =1 ; goto out;
@@ -794,6 +800,7 @@ out:if (retval != sdkp->previous_state)sdev_evt_send_simple (sdp,SDEV_EVT_MEDIA_CHANGE ,GFP_KERNEL ); sdkp->previous_state = retval; +kfree (sshdr);return retval; }
drivers/scsi/sr.c +13 -6
@@ -179,18 +179,24 @@ static int sr_media_change(struct cdrom_device_info *cdi, int slot) {struct scsi_cd *cd = cdi->handle;int retval; +struct scsi_sense_hdr *sshdr;if (CDSL_CURRENT != slot) {/* no changer support */ return -EINVAL ; } - retval =scsi_test_unit_ready (cd->device,SR_TIMEOUT ,MAX_RETRIES ); -if (retval) { -/* Unable to test, unit probably not ready. This usually -* means there is no disc in the drive. Mark as changed, -* and we will figure it out later once the drive is -* available again. */ + sshdr =kzalloc (sizeof (*sshdr),GFP_KERNEL ); + retval =scsi_test_unit_ready (cd->device,SR_TIMEOUT ,MAX_RETRIES , + sshdr); +if (retval || (scsi_sense_valid (sshdr) && +/* 0x3a is medium not present */ + sshdr->asc ==0x3a )) { +/* Media not present or unable to test, unit probably not +* ready. This usually means there is no disc in the drive. +* Mark as changed, and we will figure it out later once +* the drive is available again. +*/ cd->device->changed =1 ;/* This will force a flush, if called from check_disk_change */ retval =1 ;
@@ -213,6 +219,7 @@ out:sdev_evt_send_simple (cd->device,SDEV_EVT_MEDIA_CHANGE ,GFP_KERNEL ); cd->previous_state = retval; +kfree (sshdr);return retval; }
include/scsi/scsi_device.h +1 -1
@@ -292,7 +292,7 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,struct scsi_mode_data *data, struct scsi_sense_hdr *);extern int scsi_test_unit_ready (struct scsi_device *sdev,int timeout, -int retries); +int retries,struct scsi_sense_hdr *sshdr);extern int scsi_device_set_state (struct scsi_device *sdev,enum scsi_device_state state); extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · James Bottomley
Actually, the night train is a good place to do coding. I know this compiles, but could someone check it out? It's the form I think we should do, since the ASC/ASCQ codes for esoteric conditions which we might eventually check for are different for CDs and removable discs.
This part is puzzling:
drivers/scsi/scsi_lib.c scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, · line 2053
Quoted 7 lines
drivers/scsi/scsi_lib.c +7
-0+/* try to eat the UNIT_ATTENTION if there are enough retries */ +do { + result =scsi_execute_req (sdev, cmd,DMA_NONE ,NULL ,0 , sshdr, + timeout, retries); + }while ((driver_byte (result) &DRIVER_SENSE ) && + sshdr && sshdr->sense_key ==UNIT_ATTENTION && + --retries);
For one thing, you've using retries to control both an outer loop and an inner loop -- meaning the total number of attempts could be on the order of retries**2.
For another, why do you want to swallow a UNIT_ATTENTION? Shouldn't that be up to the code calling scsi_test_unit_ready()?
Alan Stern
Quoted 9 lines · Alan Stern
Quoted 4 lines · James Bottomley
Actually, the night train is a good place to do coding. I know this compiles, but could someone check it out? It's the form I think we should do, since the ASC/ASCQ codes for esoteric conditions which we might eventually check for are different for CDs and removable discs.
This part is puzzling:
Quoted 11 lines
drivers/scsi/scsi_lib.c scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, · line 2053
Quoted 7 lines
drivers/scsi/scsi_lib.c +7
-0+/* try to eat the UNIT_ATTENTION if there are enough retries */ +do { + result =scsi_execute_req (sdev, cmd,DMA_NONE ,NULL ,0 , sshdr, + timeout, retries); + }while ((driver_byte (result) &DRIVER_SENSE ) && + sshdr && sshdr->sense_key ==UNIT_ATTENTION && + --retries);For one thing, you've using retries to control both an outer loop and an inner loop -- meaning the total number of attempts could be on the order of retries**2.
Sort of; however, there should only really be one cc/ua before the TUR goes through. Also, TUR produces almost no conditions that would be retryable in scsi_decide_disposition() and the scsi_execute sends it down with REQ_FAILFAST anyway, so it's really not used much.
Quoted 2 lines
For another, why do you want to swallow a
UNIT_ATTENTION? Shouldn't that be up to the code callingscsi_test_unit_ready()?
No, that's a bug in the old code. UA doesn't necessarily mean there's a media change. It could be asserted for a whole host of reasons. Until we get proper UA signalling, we need to resend the TUR so we get the correct media status and discard the UA (Even if the device is asserting UA for media change reasons, it will still give the correct sense code response to the following TUR).
James
Quoted 12 lines · James Bottomley
Quoted 3 lines
For one thing, you've using retries to control both an outer loop and an inner loop -- meaning the total number of attempts could be on the order of retries**2.
Sort of; however, there should only really be one cc/ua before the TUR goes through. Also, TUR produces almost no conditions that would be retryable in
scsi_decide_disposition()and thescsi_executesends it down withREQ_FAILFASTanyway, so it's really not used much.Quoted 2 lines
For another, why do you want to swallow a
UNIT_ATTENTION? Shouldn't that be up to the code callingscsi_test_unit_ready()?
Quoted 6 lines
No, that's a bug in the old code. UA doesn't necessarily mean there's a media change. It could be asserted for a whole host of reasons. Until we get proper UA signalling, we need to resend the TUR so we get the correct media status and discard the UA (Even if the device is asserting UA for media change reasons, it will still give the correct sense code response to the following TUR).
Well then, the patch appears good. I haven't been able to test it because it's based against a source tree I haven't got. Can you provide an equivalent patch against 2.6.24-rc4?
Alan Stern