removable device: can't detect disk changes

removable device: can't detect disk changes

[Giuliano Pochini]

Fujitsu magneto-optical drive, Adaptec 29160 and 
Linux Jay 2.6.26 #7 SMP Sun Aug 10 18:34:22 CEST 2008 ppc 7455, altivec supported PowerMac3,6 GNU/Linux

When I insert a disk and I mount it, scsi_test_unit_ready() is called and
the do-while loop gets sshdr->sense_key == UNIT_ATTENTION in the first
cycle and 0 in the second one. So the if below misses the UNIT_ATTENTION
and sdev->changed = 1 is not executed. At this point bad things can
happen... I'm not sure how to fix this. Any clue ?


--
Giuliano.

Re: removable device: can't detect disk changes

[Giuliano Pochini]

On Tue, 12 Aug 2008 15:08:14 +0200
Giuliano Pochini <pochini@shiny.it> wrote:

> 
> Fujitsu magneto-optical drive, Adaptec 29160 and 
> Linux Jay 2.6.26 #7 SMP Sun Aug 10 18:34:22 CEST 2008 ppc 7455, altivec supported PowerMac3,6 GNU/Linux
> 
> When I insert a disk and I mount it, scsi_test_unit_ready() is called and
> the do-while loop gets sshdr->sense_key == UNIT_ATTENTION in the first
> cycle and 0 in the second one. So the if below misses the UNIT_ATTENTION
> and sdev->changed = 1 is not executed. At this point bad things can
> happen... I'm not sure how to fix this. Any clue ?

Ok... what about this patch ?  The while() condition also checks for
NOT_READY. For removable devices sdev->changed is set when not_ready is
detected (not only if it occurs in the last cycle).
If works fine here, but since I don't know the SCSI subsystem this patch
may be wrong.


Signed-off-by: Giuliano Pochini <pochini@shiny.it>

--- scsi_lib.c__orig	2008-08-18 21:11:45.000000000 +0200
+++ scsi_lib.c	2008-08-18 21:26:56.000000000 +0200
@@ -1961,7 +1961,7 @@ scsi_test_unit_ready(struct scsi_device 
 		TEST_UNIT_READY, 0, 0, 0, 0, 0,
 	};
 	struct scsi_sense_hdr *sshdr;
-	int result;
+	int result, not_ready;
 
 	if (!sshdr_external)
 		sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
@@ -1972,23 +1972,19 @@ scsi_test_unit_ready(struct scsi_device 
 	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);
+		not_ready = (driver_byte(result) & DRIVER_SENSE) && sshdr &&
+			    ((sshdr->sense_key == UNIT_ATTENTION) ||
+			     (sshdr->sense_key == NOT_READY));
+		if (not_ready && scsi_sense_valid(sshdr) && sdev->removable)
+			sdev->changed = 1;
+	} while (not_ready && --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))) {
-			sdev->changed = 1;
+	if (not_ready && scsi_sense_valid(sshdr) && sdev->removable)
 			result = 0;
-		}
-	}
 	if (!sshdr_external)
 		kfree(sshdr);
 	return result;


--
Giuliano.

Re: removable device: can't detect disk changes

[James Bottomley]

On Mon, 2008-08-18 at 21:52 +0200, Giuliano Pochini wrote:
> On Tue, 12 Aug 2008 15:08:14 +0200
> Giuliano Pochini <pochini@shiny.it> wrote:
> 
> > 
> > Fujitsu magneto-optical drive, Adaptec 29160 and 
> > Linux Jay 2.6.26 #7 SMP Sun Aug 10 18:34:22 CEST 2008 ppc 7455, altivec supported PowerMac3,6 GNU/Linux
> > 
> > When I insert a disk and I mount it, scsi_test_unit_ready() is called and
> > the do-while loop gets sshdr->sense_key == UNIT_ATTENTION in the first
> > cycle and 0 in the second one. So the if below misses the UNIT_ATTENTION
> > and sdev->changed = 1 is not executed. At this point bad things can
> > happen... I'm not sure how to fix this. Any clue ?
> 
> Ok... what about this patch ?  The while() condition also checks for
> NOT_READY. For removable devices sdev->changed is set when not_ready is
> detected (not only if it occurs in the last cycle).
> If works fine here, but since I don't know the SCSI subsystem this patch
> may be wrong.

Well done! Almost ... apparently what needs to happen is what we're
currently doing in sr_test_unit_ready.  There's no need to keep checking
for NOT_READY ... that isn't a volatile condition like UNIT ATTENTION
and driver_byte(result) & DRIVER_SENSE is equivalent to
scsi_sense_valid(sshdr) in most cases.  Also, the while loop should exit
on a non-UNIT ATTENTION condition (like NOT READY).

Does this work?

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ff5d56b..f2b03b7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2022,22 +2022,22 @@ scsi_test_unit_ready(struct scsi_device *sdev, int timeout, int 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 &&
+		if (sdev->removable && sshdr && scsi_sense_valid(sshdr) &&
+		    sshdr->sense_key == UNIT_ATTENTION)
+			sdev->changed = 1;
+	} while (sshdr && scsi_sense_valid(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))) {
-			sdev->changed = 1;
-			result = 0;
-		}
+	if (sdev->removable && scsi_sense_valid(sshdr) &&
+	    (sshdr->sense_key == UNIT_ATTENTION ||
+	     sshdr->sense_key == NOT_READY)) {
+		sdev->changed = 1;
+		result = 0;
 	}
 	if (!sshdr_external)
 		kfree(sshdr);

Re: removable device: can't detect disk changes

[Giuliano Pochini]

On Mon, 18 Aug 2008 15:58:14 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Mon, 2008-08-18 at 21:52 +0200, Giuliano Pochini wrote:
> > On Tue, 12 Aug 2008 15:08:14 +0200
> > Giuliano Pochini <pochini@shiny.it> wrote:
> > 
> > > 
> > > Fujitsu magneto-optical drive, Adaptec 29160 and 
> > > Linux Jay 2.6.26 #7 SMP Sun Aug 10 18:34:22 CEST 2008 ppc 7455, altivec supported PowerMac3,6 GNU/Linux
> > > 
> > > When I insert a disk and I mount it, scsi_test_unit_ready() is called and
> > > the do-while loop gets sshdr->sense_key == UNIT_ATTENTION in the first
> > > cycle and 0 in the second one. So the if below misses the UNIT_ATTENTION
> > > and sdev->changed = 1 is not executed. At this point bad things can
> > > happen... I'm not sure how to fix this. Any clue ?
> > 
> > Ok... what about this patch ?  The while() condition also checks for
> > NOT_READY. For removable devices sdev->changed is set when not_ready is
> > detected (not only if it occurs in the last cycle).
> > If works fine here, but since I don't know the SCSI subsystem this patch
> > may be wrong.
> 
> Well done! Almost ... apparently what needs to happen is what we're
> currently doing in sr_test_unit_ready.  There's no need to keep checking
> for NOT_READY ... that isn't a volatile condition like UNIT ATTENTION
> and driver_byte(result) & DRIVER_SENSE is equivalent to
> scsi_sense_valid(sshdr) in most cases.  Also, the while loop should exit
> on a non-UNIT ATTENTION condition (like NOT READY).
>
> Does this work?

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
[...]

Yes, it works fine. This is your patch without the check for sshdr!=NULL
because it is already in scsi_sense_valid().


--- drivers/scsi/scsi_lib.c__orig	2008-08-18 21:11:45.000000000 +0200
+++ drivers/scsi/scsi_lib.c	2008-08-19 22:31:41.000000000 +0200
@@ -1972,22 +1972,21 @@ scsi_test_unit_ready(struct scsi_device 
 	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 (sdev->removable && scsi_sense_valid(sshdr) &&
+		    sshdr->sense_key == UNIT_ATTENTION)
+			sdev->changed = 1;
+	} while (scsi_sense_valid(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))) {
-			sdev->changed = 1;
-			result = 0;
-		}
+	if (sdev->removable && scsi_sense_valid(sshdr) &&
+	    (sshdr->sense_key == UNIT_ATTENTION ||
+	     sshdr->sense_key == NOT_READY)) {
+		sdev->changed = 1;
+		result = 0;
 	}
 	if (!sshdr_external)
 		kfree(sshdr);



--
Giuliano.

Re: removable device: can't detect disk changes

[Giuliano Pochini]

On Wed, 20 Aug 2008 00:58:13 +0200
Giuliano Pochini <pochini@shiny.it> wrote:

> On Mon, 18 Aug 2008 15:58:14 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Mon, 2008-08-18 at 21:52 +0200, Giuliano Pochini wrote:
> > > On Tue, 12 Aug 2008 15:08:14 +0200
> > > Giuliano Pochini <pochini@shiny.it> wrote:
> > > 
> > > > 
> > > > Fujitsu magneto-optical drive, Adaptec 29160 and 
> > > > Linux Jay 2.6.26 #7 SMP Sun Aug 10 18:34:22 CEST 2008 ppc 7455, altivec supported PowerMac3,6 GNU/Linux
> > > > 
> > > > When I insert a disk and I mount it, scsi_test_unit_ready() is called and
> > > > the do-while loop gets sshdr->sense_key == UNIT_ATTENTION in the first
> > > > cycle and 0 in the second one. So the if below misses the UNIT_ATTENTION
> > > > and sdev->changed = 1 is not executed. At this point bad things can
> > > > happen... I'm not sure how to fix this. Any clue ?
> > > 
> > > Ok... what about this patch ?  The while() condition also checks for
> > > NOT_READY. For removable devices sdev->changed is set when not_ready is
> > > detected (not only if it occurs in the last cycle).
> > > If works fine here, but since I don't know the SCSI subsystem this patch
> > > may be wrong.
> > 
> > Well done! Almost ... apparently what needs to happen is what we're
> > currently doing in sr_test_unit_ready.  There's no need to keep checking
> > for NOT_READY ... that isn't a volatile condition like UNIT ATTENTION
> > and driver_byte(result) & DRIVER_SENSE is equivalent to
> > scsi_sense_valid(sshdr) in most cases.  Also, the while loop should exit
> > on a non-UNIT ATTENTION condition (like NOT READY).
> >
> > Does this work?
> 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> [...]
> 
> Yes, it works fine. This is your patch without the check for sshdr!=NULL
> because it is already in scsi_sense_valid().
> 
> 
> --- drivers/scsi/scsi_lib.c__orig	2008-08-18 21:11:45.000000000 +0200
> +++ drivers/scsi/scsi_lib.c	2008-08-19 22:31:41.000000000 +0200
> @@ -1972,22 +1972,21 @@ scsi_test_unit_ready(struct scsi_device 
>  	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 (sdev->removable && scsi_sense_valid(sshdr) &&
> +		    sshdr->sense_key == UNIT_ATTENTION)
> +			sdev->changed = 1;
> +	} while (scsi_sense_valid(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))) {
> -			sdev->changed = 1;
> -			result = 0;
> -		}
> +	if (sdev->removable && scsi_sense_valid(sshdr) &&
> +	    (sshdr->sense_key == UNIT_ATTENTION ||
> +	     sshdr->sense_key == NOT_READY)) {
> +		sdev->changed = 1;
> +		result = 0;
>  	}
>  	if (!sshdr_external)
>  		kfree(sshdr);

Will you merge this patch ? If you think there is something wrong, please
send me a new patch ASAP. I'm going to sell my removable drive, thus I'll
not be able to test it anymore.


--
Giuliano.