public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
* [patch] SCSI: early detection of medium not present, updated
@ 2007-11-29 23:18 Andrew Morton
  2007-11-30  0:04 ` Tejun Heo
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Andrew Morton @ 2007-11-29 23:18 UTC (permalink / raw)
  To: James Bottomley, linux-scsi, Albert Lee, Alan Stern, Tejun Heo,
	Je


Guys, I have this marked as needed-in-2.6.24?





From: Alan Stern <stern@rowland.harvard.edu>

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.

Cc: Albert Lee <albertcc@tw.ibm.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: James Bottomley <James.Bottomley@steeleye.com>
Cc: Tejun Heo <htejun@gmail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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(-)

diff -puN drivers/scsi/scsi_ioctl.c~scsi-early-detection-of-medium-not-present-updated drivers/scsi/scsi_ioctl.c
--- a/drivers/scsi/scsi_ioctl.c~scsi-early-detection-of-medium-not-present-updated
+++ a/drivers/scsi/scsi_ioctl.c
@@ -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;
diff -puN drivers/scsi/scsi_lib.c~scsi-early-detection-of-medium-not-present-updated drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c~scsi-early-detection-of-medium-not-present-updated
+++ a/drivers/scsi/scsi_lib.c
@@ -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_device 
 		if ((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);
diff -puN drivers/scsi/sd.c~scsi-early-detection-of-medium-not-present-updated drivers/scsi/sd.c
--- a/drivers/scsi/sd.c~scsi-early-detection-of-medium-not-present-updated
+++ a/drivers/scsi/sd.c
@@ -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
diff -puN drivers/scsi/sr.c~scsi-early-detection-of-medium-not-present-updated drivers/scsi/sr.c
--- a/drivers/scsi/sr.c~scsi-early-detection-of-medium-not-present-updated
+++ a/drivers/scsi/sr.c
@@ -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;
diff -puN include/scsi/scsi_device.h~scsi-early-detection-of-medium-not-present-updated include/scsi/scsi_device.h
--- a/include/scsi/scsi_device.h~scsi-early-detection-of-medium-not-present-updated
+++ a/include/scsi/scsi_device.h
@@ -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,
_


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-11-29 23:18 [patch] SCSI: early detection of medium not present, updated Andrew Morton
@ 2007-11-30  0:04 ` Tejun Heo
  2007-11-30  3:26 ` Alan Stern
  2007-12-01 14:24 ` James Bottomley
  2 siblings, 0 replies; 9+ messages in thread
From: Tejun Heo @ 2007-11-30  0:04 UTC (permalink / raw)
  To: Andrew Morton
  Cc: James Bottomley, linux-scsi, Albert Lee, Alan Stern, Jens Axboe

Andrew Morton wrote:
> From: Alan Stern <stern@rowland.harvard.edu>
> 
> 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.
> 
> Cc: Albert Lee <albertcc@tw.ibm.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: James Bottomley <James.Bottomley@steeleye.com>
> Cc: Tejun Heo <htejun@gmail.com>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

I think I did it last time but...

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-11-29 23:18 [patch] SCSI: early detection of medium not present, updated Andrew Morton
  2007-11-30  0:04 ` Tejun Heo
@ 2007-11-30  3:26 ` Alan Stern
  2007-12-01 14:24 ` James Bottomley
  2 siblings, 0 replies; 9+ messages in thread
From: Alan Stern @ 2007-11-30  3:26 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Joe Reid, James Bottomley, SCSI development list, Albert Lee,
	Tejun Heo, Jens Axboe

On Thu, 29 Nov 2007, Andrew Morton wrote:

> Guys, I have this marked as needed-in-2.6.24?

> From: Alan Stern <stern@rowland.harvard.edu>
> 
> 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.

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-11-29 23:18 [patch] SCSI: early detection of medium not present, updated Andrew Morton
  2007-11-30  0:04 ` Tejun Heo
  2007-11-30  3:26 ` Alan Stern
@ 2007-12-01 14:24 ` James Bottomley
  2007-12-01 15:55   ` Alan Stern
  2007-12-02 17:10   ` James Bottomley
  2 siblings, 2 replies; 9+ messages in thread
From: James Bottomley @ 2007-12-01 14:24 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-scsi, Albert Lee, Alan Stern, Tejun Heo, Jens Axboe


On Thu, 2007-11-29 at 15:18 -0800, Andrew Morton wrote:
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-12-01 14:24 ` James Bottomley
@ 2007-12-01 15:55   ` Alan Stern
  2007-12-02 17:10   ` James Bottomley
  1 sibling, 0 replies; 9+ messages in thread
From: Alan Stern @ 2007-12-01 15:55 UTC (permalink / raw)
  To: James Bottomley
  Cc: Andrew Morton, linux-scsi, Albert Lee, Tejun Heo, Jens Axboe

On Sat, 1 Dec 2007, James Bottomley wrote:

> On Thu, 2007-11-29 at 15:18 -0800, Andrew Morton wrote:
> > 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.

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-12-01 14:24 ` James Bottomley
  2007-12-01 15:55   ` Alan Stern
@ 2007-12-02 17:10   ` James Bottomley
  2007-12-02 20:02     ` Alan Stern
  1 sibling, 1 reply; 9+ messages in thread
From: James Bottomley @ 2007-12-02 17:10 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-scsi, Albert Lee, Alan Stern, Tejun Heo, Jens Axboe

On Sat, 2007-12-01 at 16:25 +0200, James Bottomley wrote:
> On Thu, 2007-11-29 at 15:18 -0800, Andrew Morton wrote:
> > 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.

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

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 83e1447..28b19ef 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -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;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1148c40..823df63 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2016,27 +2016,57 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 }
 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
+ *	@sshdr_external: Optional pointer to struct scsi_sense_hdr for
+ *		returning sense. Make sure that this is cleared before passing
+ *		in.
+ *
+ *	Returns zero if unsuccessful or an error if TUR failed.  For
+ *	removable media, a return of NOT_READY or UNIT_ATTENTION is
+ *	translated to success, with the ->changed flag updated.
+ **/
 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,
+		     struct scsi_sense_hdr *sshdr_external)
 {
 	char cmd[] = {
 		TEST_UNIT_READY, 0, 0, 0, 0, 0,
 	};
-	struct scsi_sense_hdr sshdr;
+	struct scsi_sense_hdr *sshdr;
 	int result;
-	
-	result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0, &sshdr,
-				  timeout, retries);
+
+	if (!sshdr_external)
+		sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
+	else
+		sshdr = sshdr_external;
+
+	/* 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);
+
+	if (!sshdr)
+		/* could not allocate sense buffer, so can't process it */
+		return result;
 
 	if ((driver_byte(result) & DRIVER_SENSE) && sdev->removable) {
 
-		if ((scsi_sense_valid(&sshdr)) &&
-		    ((sshdr.sense_key == UNIT_ATTENTION) ||
-		     (sshdr.sense_key == NOT_READY))) {
+		if ((scsi_sense_valid(sshdr)) &&
+		    ((sshdr->sense_key == UNIT_ATTENTION) ||
+		     (sshdr->sense_key == NOT_READY))) {
 			sdev->changed = 1;
 			result = 0;
 		}
 	}
+	if (!sshdr_external)
+		kfree(sshdr);
 	return result;
 }
 EXPORT_SYMBOL(scsi_test_unit_ready);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 18343a6..212f6bc 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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;
 }
 
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7702681..896be4a 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -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;
 }
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c7bccf9..b1861c7 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-12-02 17:10   ` James Bottomley
@ 2007-12-02 20:02     ` Alan Stern
  2007-12-05 16:06       ` James Bottomley
  0 siblings, 1 reply; 9+ messages in thread
From: Alan Stern @ 2007-12-02 20:02 UTC (permalink / raw)
  To: James Bottomley
  Cc: Andrew Morton, linux-scsi, Albert Lee, Tejun Heo, Jens Axboe

On Sun, 2 Dec 2007, James Bottomley wrote:

> 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:

> +	/* 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-12-02 20:02     ` Alan Stern
@ 2007-12-05 16:06       ` James Bottomley
  2007-12-07 15:51         ` Alan Stern
  0 siblings, 1 reply; 9+ messages in thread
From: James Bottomley @ 2007-12-05 16:06 UTC (permalink / raw)
  To: Alan Stern; +Cc: Andrew Morton, linux-scsi, Albert Lee, Tejun Heo, Jens Axboe


On Sun, 2007-12-02 at 15:02 -0500, Alan Stern wrote:
> On Sun, 2 Dec 2007, James Bottomley wrote:
> 
> > 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:
> 
> > +	/* 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.

> For another, why do you want to swallow a UNIT_ATTENTION?  Shouldn't 
> that be up to the code calling scsi_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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch] SCSI: early detection of medium not present, updated
  2007-12-05 16:06       ` James Bottomley
@ 2007-12-07 15:51         ` Alan Stern
  0 siblings, 0 replies; 9+ messages in thread
From: Alan Stern @ 2007-12-07 15:51 UTC (permalink / raw)
  To: James Bottomley
  Cc: Andrew Morton, linux-scsi, Albert Lee, Tejun Heo, Jens Axboe

On Wed, 5 Dec 2007, James Bottomley wrote:

> > 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.
> 
> > For another, why do you want to swallow a UNIT_ATTENTION?  Shouldn't 
> > that be up to the code calling scsi_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).

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2007-12-07 15:51 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-29 23:18 [patch] SCSI: early detection of medium not present, updated Andrew Morton
2007-11-30  0:04 ` Tejun Heo
2007-11-30  3:26 ` Alan Stern
2007-12-01 14:24 ` James Bottomley
2007-12-01 15:55   ` Alan Stern
2007-12-02 17:10   ` James Bottomley
2007-12-02 20:02     ` Alan Stern
2007-12-05 16:06       ` James Bottomley
2007-12-07 15:51         ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox