[regression] CD-DA delay needed after insertion

[regression] CD-DA delay needed after insertion

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1195 bytes --]

We've found another regression in 2.6.25 w.r.t. CD media change on PS3.

It can easily be reproduced by:

  1. Inserting an audio CD
  2. Running the following command as soon as the blue CD/DVD/BD drive LED
     stops blinking and is lit continuously:
      
	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed

On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
insertion, or it will fail.
On 2.6.24 and older, it just works immediately.

It does not matter whether
http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
is applied or not.

We haven't bisected it yet.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Technology and Software Centre Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: [regression] CD-DA delay needed after insertion

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> 
> It can easily be reproduced by:
> 
>   1. Inserting an audio CD
>   2. Running the following command as soon as the blue CD/DVD/BD drive LED
>      stops blinking and is lit continuously:
>       
> 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> 
> On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> insertion, or it will fail.
> On 2.6.24 and older, it just works immediately.
> 
> It does not matter whether
> http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> is applied or not.
> 
> We haven't bisected it yet.
> 
> With kind regards,
> 
> Geert Uytterhoeven
> Software Architect
> 
> Sony Techsoft Centre
> The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
> 
> Phone:    +32 (0)2 700 8453
> Fax:      +32 (0)2 700 8622
> E-mail:   Geert.Uytterhoeven@sonycom.com
> Internet: http://www.sony-europe.com/
> 
> Sony Technology and Software Centre Europe
> A division of Sony Service Centre (Europe) N.V.
> Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
> VAT BE 0413.825.160 · RPR Brussels
> Fortis 293-0376800-10 GEBA-BE-BB

James hi.

It looks like the same problem as before. Any BLOCK_PC command will 
eat the UNIT_ATTENTION notification. Which is what I was afraid of. 
I was thinking of something like below.

Totally untested, never even booted. It is just to demonstrate the concept.
Please tell me if it is at all desirable and I'll spend some time to see
if it runs, and test it.

Boaz
---
From 8cd166b76100a2830f0f973df866f5509398f6cc Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Tue, 10 Jun 2008 20:09:43 +0300
Subject: [PATCH] scsi: Proccess UNIT_ATTENTION on any type command

Let scsi_check_sense() report UNIT_ATTENTION media
changes for any type of command. Currently only FS
commands are reported.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |    4 ++++
 drivers/scsi/scsi_lib.c   |    3 +--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 006a959..7039d11 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -370,6 +370,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		if (scmd->device->allow_restart &&
 		    (sshdr.asc == 0x04) && (sshdr.ascq == 0x02))
 			return FAILED;
+
+		/* Detected disc change.*/
+		if (scmd->device->removable)
+			scmd->device->changed = 1;
 		return SUCCESS;
 
 		/* these three are not supported */
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 033c58a..efd3e09 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -903,11 +903,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	if (sense_valid && !sense_deferred) {
 		switch (sshdr.sense_key) {
 		case UNIT_ATTENTION:
-			if (cmd->device->removable) {
+			if (cmd->device->changed) {
 				/* Detected disc change.  Set a bit
 				 * and quietly refuse further access.
 				 */
-				cmd->device->changed = 1;
 				scsi_end_request(cmd, -EIO, this_count, 1);
 				return;
 			} else {
-- 
1.5.6.rc1.5.gadf6

Re: [regression] CD-DA delay needed after insertion

[James Bottomley]

On Sun, 2008-06-15 at 15:46 +0300, Boaz Harrosh wrote:
> Geert Uytterhoeven wrote:
> > We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> > 
> > It can easily be reproduced by:
> > 
> >   1. Inserting an audio CD
> >   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> >      stops blinking and is lit continuously:
> >       
> > 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> > 
> > On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> > insertion, or it will fail.
> > On 2.6.24 and older, it just works immediately.
> > 
> > It does not matter whether
> > http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> > is applied or not.
> > 
> > We haven't bisected it yet.
> > 
> > With kind regards,
> > 
> > Geert Uytterhoeven
> > Software Architect
> > 
> > Sony Techsoft Centre
> > The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
> > 
> > Phone:    +32 (0)2 700 8453
> > Fax:      +32 (0)2 700 8622
> > E-mail:   Geert.Uytterhoeven@sonycom.com
> > Internet: http://www.sony-europe.com/
> > 
> > Sony Technology and Software Centre Europe
> > A division of Sony Service Centre (Europe) N.V.
> > Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
> > VAT BE 0413.825.160 · RPR Brussels
> > Fortis 293-0376800-10 GEBA-BE-BB
> 
> James hi.
> 
> It looks like the same problem as before. Any BLOCK_PC command will 
> eat the UNIT_ATTENTION notification. Which is what I was afraid of. 
> I was thinking of something like below.

It's a bit unlikely to be the cause of a regression, since the code has
behaved this way by design since at least 2.4.  The problem we're
looking for appeared in 2.6.25+

Also, the taxonomy of the regression: errors returned within 10s of
inserting the CD sounds very much like an ignored NOT_READY.

> Totally untested, never even booted. It is just to demonstrate the concept.
> Please tell me if it is at all desirable and I'll spend some time to see
> if it runs, and test it.

Really, no ... not until there's a proveable problem caused by it.  Even
then, not like this: the way you've coded it interferes with the cc_ua
processing code.  It would have to go in scsi_decide_disposition().

James

Re: [regression] CD-DA delay needed after insertion

[James Bottomley]

On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> 
> It can easily be reproduced by:
> 
>   1. Inserting an audio CD
>   2. Running the following command as soon as the blue CD/DVD/BD drive LED
>      stops blinking and is lit continuously:
>       
> 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> 
> On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> insertion, or it will fail.
> On 2.6.24 and older, it just works immediately.
> 
> It does not matter whether
> http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> is applied or not.
> 
> We haven't bisected it yet.

There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
so I'd bet on the previous culprits.

This time, the taxonomy looks like NOT_READY isn't being waited for
properly.  I'd still tend to blame
210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
suspect this to be the problem line:

	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
		return CDS_DISC_OK;

Previously the code would have returned CDS_NO_DISC for the not ready
case.  The CD would have tried to close the door but (and this is the
key) it would have waited for the door to close before trying again.

James

Re: [regression] CD-DA delay needed after insertion

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4045 bytes --]

	Hi James,

On Sun, 15 Jun 2008, James Bottomley wrote:
> On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> > We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> > 
> > It can easily be reproduced by:
> > 
> >   1. Inserting an audio CD
> >   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> >      stops blinking and is lit continuously:
> >       
> > 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> > 
> > On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> > insertion, or it will fail.
> > On 2.6.24 and older, it just works immediately.
> > 
> > It does not matter whether
> > http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> > is applied or not.
> > 
> > We haven't bisected it yet.
> 
> There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> so I'd bet on the previous culprits.
> 
> This time, the taxonomy looks like NOT_READY isn't being waited for
> properly.  I'd still tend to blame
> 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I

Reverting that commit doesn't fix the problem.

> suspect this to be the problem line:
> 
> 	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> 		return CDS_DISC_OK;
> 
> Previously the code would have returned CDS_NO_DISC for the not ready
> case.  The CD would have tried to close the door but (and this is the
> key) it would have waited for the door to close before trying again.

I tried the patch below.

Now the kernel spits out messages every 2 seconds:

| G: CDS_NO_DISC
| G: CDS_NO_DISC

Insert CD-DA media

Reading from CD using cdparanioa fails

| F: CDS_DISC_OK
| F: CDS_DISC_OK
| F: CDS_DISC_OK
| F: CDS_DISC_OK
| F: CDS_DISC_OK

Reading from CD starts to work here

| B: CDS_DISC_OK
| B: CDS_DISC_OK

I tried changing the return value in the `F' case to CDS_NO_DISC, but that
doesn't make a difference.

--- a/drivers/scsi/sr_ioctl.c	2008-06-16 16:34:01.000000000 +0200
+++ b/drivers/scsi/sr_ioctl.c	2008-06-16 16:59:28.000000000 +0200
@@ -304,25 +304,34 @@ int sr_drive_status(struct cdrom_device_
 
 	if (CDSL_CURRENT != slot) {
 		/* we have no changer support */
+printk("A: -EINVAL\n");
 		return -EINVAL;
 	}
-	if (0 == sr_test_unit_ready(cd->device, &sshdr))
+	if (0 == sr_test_unit_ready(cd->device, &sshdr)) {
+printk("B: CDS_DISC_OK\n");
 		return CDS_DISC_OK;
+	}
 
 	if (!cdrom_get_media_event(cdi, &med)) {
-		if (med.media_present)
+		if (med.media_present) {
+printk("C: CDS_DISC_OK\n");
 			return CDS_DISC_OK;
-		else if (med.door_open)
+		} else if (med.door_open) {
+printk("D: CDS_TRAY_OPEN\n");
 			return CDS_TRAY_OPEN;
-		else
+		} else {
+printk("E: CDS_NO_DISC\n");
 			return CDS_NO_DISC;
+		}
 	}
 
 	/*
 	 * 0x04 is format in progress .. but there must be a disc present!
 	 */
-	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
+	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04) {
+printk("F: CDS_DISC_OK\n");
 		return CDS_DISC_OK;
+	}
 
 	/*
 	 * If not using Mt Fuji extended media tray reports,
@@ -331,11 +340,15 @@ int sr_drive_status(struct cdrom_device_
 	 */
 	if (scsi_sense_valid(&sshdr) &&
 	    /* 0x3a is medium not present */
-	    sshdr.asc == 0x3a)
+	    sshdr.asc == 0x3a) {
+printk("G: CDS_NO_DISC\n");
 		return CDS_NO_DISC;
-	else
+	} else {
+printk("H: CDS_TRAY_OPEN\n");
 		return CDS_TRAY_OPEN;
+	}
 
+printk("I: CDS_DRIVE_NOT_READY\n");
 	return CDS_DRIVE_NOT_READY;
 }
 

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Technology and Software Centre Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: [regression] CD-DA delay needed after insertion

[James Bottomley]

On Mon, 2008-06-16 at 17:05 +0200, Geert Uytterhoeven wrote:
> 	Hi James,
> 
> On Sun, 15 Jun 2008, James Bottomley wrote:
> > On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> > > We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> > > 
> > > It can easily be reproduced by:
> > > 
> > >   1. Inserting an audio CD
> > >   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> > >      stops blinking and is lit continuously:
> > >       
> > > 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> > > 
> > > On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> > > insertion, or it will fail.
> > > On 2.6.24 and older, it just works immediately.
> > > 
> > > It does not matter whether
> > > http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> > > is applied or not.
> > > 
> > > We haven't bisected it yet.
> > 
> > There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> > so I'd bet on the previous culprits.
> > 
> > This time, the taxonomy looks like NOT_READY isn't being waited for
> > properly.  I'd still tend to blame
> > 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
> 
> Reverting that commit doesn't fix the problem.

That's a bit of a problem.  Particularly as:

> > suspect this to be the problem line:
> > 
> > 	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> > 		return CDS_DISC_OK;
> > 
> > Previously the code would have returned CDS_NO_DISC for the not ready
> > case.  The CD would have tried to close the door but (and this is the
> > key) it would have waited for the door to close before trying again.
> 
> I tried the patch below.
> 
> Now the kernel spits out messages every 2 seconds:
> 
> | G: CDS_NO_DISC
> | G: CDS_NO_DISC
> 
> Insert CD-DA media
> 
> Reading from CD using cdparanioa fails
> 
> | F: CDS_DISC_OK
> | F: CDS_DISC_OK
> | F: CDS_DISC_OK
> | F: CDS_DISC_OK
> | F: CDS_DISC_OK

That's in the two lines I fingered as the source of the problem ...
however, if reversing the apparently bad commit (that introduces those
lines) doesn't work there's something else wrong.

Could you try altering your F: case to return CDS_DRIVE_NOT_READY, but
if that doesn't work, it's going to take a bit of bisection, I'm
afraid ...

Thanks,

James


> Reading from CD starts to work here
> 
> | B: CDS_DISC_OK
> | B: CDS_DISC_OK
> 
> I tried changing the return value in the `F' case to CDS_NO_DISC, but that
> doesn't make a difference.
> 
> --- a/drivers/scsi/sr_ioctl.c	2008-06-16 16:34:01.000000000 +0200
> +++ b/drivers/scsi/sr_ioctl.c	2008-06-16 16:59:28.000000000 +0200
> @@ -304,25 +304,34 @@ int sr_drive_status(struct cdrom_device_
>  
>  	if (CDSL_CURRENT != slot) {
>  		/* we have no changer support */
> +printk("A: -EINVAL\n");
>  		return -EINVAL;
>  	}
> -	if (0 == sr_test_unit_ready(cd->device, &sshdr))
> +	if (0 == sr_test_unit_ready(cd->device, &sshdr)) {
> +printk("B: CDS_DISC_OK\n");
>  		return CDS_DISC_OK;
> +	}
>  
>  	if (!cdrom_get_media_event(cdi, &med)) {
> -		if (med.media_present)
> +		if (med.media_present) {
> +printk("C: CDS_DISC_OK\n");
>  			return CDS_DISC_OK;
> -		else if (med.door_open)
> +		} else if (med.door_open) {
> +printk("D: CDS_TRAY_OPEN\n");
>  			return CDS_TRAY_OPEN;
> -		else
> +		} else {
> +printk("E: CDS_NO_DISC\n");
>  			return CDS_NO_DISC;
> +		}
>  	}
>  
>  	/*
>  	 * 0x04 is format in progress .. but there must be a disc present!
>  	 */
> -	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> +	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04) {
> +printk("F: CDS_DISC_OK\n");
>  		return CDS_DISC_OK;
> +	}
>  
>  	/*
>  	 * If not using Mt Fuji extended media tray reports,
> @@ -331,11 +340,15 @@ int sr_drive_status(struct cdrom_device_
>  	 */
>  	if (scsi_sense_valid(&sshdr) &&
>  	    /* 0x3a is medium not present */
> -	    sshdr.asc == 0x3a)
> +	    sshdr.asc == 0x3a) {
> +printk("G: CDS_NO_DISC\n");
>  		return CDS_NO_DISC;
> -	else
> +	} else {
> +printk("H: CDS_TRAY_OPEN\n");
>  		return CDS_TRAY_OPEN;
> +	}
>  
> +printk("I: CDS_DRIVE_NOT_READY\n");
>  	return CDS_DRIVE_NOT_READY;
>  }
>  
> 
> With kind regards,
> 
> Geert Uytterhoeven
> Software Architect
> 
> Sony Techsoft Centre
> The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
> 
> Phone:    +32 (0)2 700 8453
> Fax:      +32 (0)2 700 8622
> E-mail:   Geert.Uytterhoeven@sonycom.com
> Internet: http://www.sony-europe.com/
> 
> Sony Technology and Software Centre Europe
> A division of Sony Service Centre (Europe) N.V.
> Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
> VAT BE 0413.825.160 · RPR Brussels
> Fortis 293-0376800-10 GEBA-BE-BB

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

Re: [regression] CD-DA delay needed after insertion

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3648 bytes --]

On Mon, 16 Jun 2008, James Bottomley wrote:
> On Mon, 2008-06-16 at 17:05 +0200, Geert Uytterhoeven wrote:
> > On Sun, 15 Jun 2008, James Bottomley wrote:
> > > On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> > > > We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> > > > 
> > > > It can easily be reproduced by:
> > > > 
> > > >   1. Inserting an audio CD
> > > >   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> > > >      stops blinking and is lit continuously:
> > > >       
> > > > 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> > > > 
> > > > On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> > > > insertion, or it will fail.
> > > > On 2.6.24 and older, it just works immediately.
> > > > 
> > > > It does not matter whether
> > > > http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> > > > is applied or not.
> > > > 
> > > > We haven't bisected it yet.
> > > 
> > > There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> > > so I'd bet on the previous culprits.
> > > 
> > > This time, the taxonomy looks like NOT_READY isn't being waited for
> > > properly.  I'd still tend to blame
> > > 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
> > 
> > Reverting that commit doesn't fix the problem.
> 
> That's a bit of a problem.  Particularly as:
> 
> > > suspect this to be the problem line:
> > > 
> > > 	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> > > 		return CDS_DISC_OK;
> > > 
> > > Previously the code would have returned CDS_NO_DISC for the not ready
> > > case.  The CD would have tried to close the door but (and this is the
> > > key) it would have waited for the door to close before trying again.
> > 
> > I tried the patch below.
> > 
> > Now the kernel spits out messages every 2 seconds:
> > 
> > | G: CDS_NO_DISC
> > | G: CDS_NO_DISC
> > 
> > Insert CD-DA media
> > 
> > Reading from CD using cdparanioa fails
> > 
> > | F: CDS_DISC_OK
> > | F: CDS_DISC_OK
> > | F: CDS_DISC_OK
> > | F: CDS_DISC_OK
> > | F: CDS_DISC_OK
> 
> That's in the two lines I fingered as the source of the problem ...
> however, if reversing the apparently bad commit (that introduces those
> lines) doesn't work there's something else wrong.
> 
> Could you try altering your F: case to return CDS_DRIVE_NOT_READY, but
> if that doesn't work, it's going to take a bit of bisection, I'm
> afraid ...

Doesn't make any difference.

git-bisect taught me it was introduced by

commit 38582a62ecd337de4212004c7d4844899dc57890
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Wed Feb 6 13:01:58 2008 -0600

    [SCSI] sr: fix test unit ready responses
    
    Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
    the scsi_test_unit_ready() function.  Unfortunately, this has the
    wrong characteristic of eating NOT_READY returns which sr.c relies on
    for tray status.
    
    Fix by rolling an internal sr_test_unit_ready() that doesn't do this.


With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Technology and Software Centre Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: [regression] CD-DA delay needed after insertion

[James Bottomley]

On Tue, 2008-06-17 at 15:31 +0200, Geert Uytterhoeven wrote:
> On Mon, 16 Jun 2008, James Bottomley wrote:
> > On Mon, 2008-06-16 at 17:05 +0200, Geert Uytterhoeven wrote:
> > > On Sun, 15 Jun 2008, James Bottomley wrote:
> > > > On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> > > > > We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> > > > > 
> > > > > It can easily be reproduced by:
> > > > > 
> > > > >   1. Inserting an audio CD
> > > > >   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> > > > >      stops blinking and is lit continuously:
> > > > >       
> > > > > 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> > > > > 
> > > > > On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> > > > > insertion, or it will fail.
> > > > > On 2.6.24 and older, it just works immediately.
> > > > > 
> > > > > It does not matter whether
> > > > > http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> > > > > is applied or not.
> > > > > 
> > > > > We haven't bisected it yet.
> > > > 
> > > > There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> > > > so I'd bet on the previous culprits.
> > > > 
> > > > This time, the taxonomy looks like NOT_READY isn't being waited for
> > > > properly.  I'd still tend to blame
> > > > 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
> > > 
> > > Reverting that commit doesn't fix the problem.
> > 
> > That's a bit of a problem.  Particularly as:
> > 
> > > > suspect this to be the problem line:
> > > > 
> > > > 	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> > > > 		return CDS_DISC_OK;
> > > > 
> > > > Previously the code would have returned CDS_NO_DISC for the not ready
> > > > case.  The CD would have tried to close the door but (and this is the
> > > > key) it would have waited for the door to close before trying again.
> > > 
> > > I tried the patch below.
> > > 
> > > Now the kernel spits out messages every 2 seconds:
> > > 
> > > | G: CDS_NO_DISC
> > > | G: CDS_NO_DISC
> > > 
> > > Insert CD-DA media
> > > 
> > > Reading from CD using cdparanioa fails
> > > 
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > 
> > That's in the two lines I fingered as the source of the problem ...
> > however, if reversing the apparently bad commit (that introduces those
> > lines) doesn't work there's something else wrong.
> > 
> > Could you try altering your F: case to return CDS_DRIVE_NOT_READY, but
> > if that doesn't work, it's going to take a bit of bisection, I'm
> > afraid ...
> 
> Doesn't make any difference.
> 
> git-bisect taught me it was introduced by
> 
> commit 38582a62ecd337de4212004c7d4844899dc57890
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Wed Feb 6 13:01:58 2008 -0600
> 
>     [SCSI] sr: fix test unit ready responses
>     
>     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
>     the scsi_test_unit_ready() function.  Unfortunately, this has the
>     wrong characteristic of eating NOT_READY returns which sr.c relies on
>     for tray status.
>     
>     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.

OK, that basically just replaced scsi_test_unit_ready with
sr_test_unit_ready.  The only difference I can see is that
scsi_test_unit_ready treats NOT_READY as a media change event and the
new code doesn't.  This seems slightly wrong:  NOT_READY can indeed mean
I have no medium, which is obviously what it's looking for.  However, it
can also mean there's another command in progress.  If this is some type
of write or format, you don't necessarily want the cd treating it as a
disk change.

However, before we get into serious debugging, could you try this patch
(applies over the previous fix for the capacity problem)?  I think it
should restore the old not ready == medium change behaviour.

Thanks,

James

---
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index c82df8b..a9acbc8 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -179,7 +179,8 @@ int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
 					      0, sshdr, SR_TIMEOUT,
 					      retries--);
 		if (scsi_sense_valid(sshdr) &&
-		    sshdr->sense_key == UNIT_ATTENTION)
+		    (sshdr->sense_key == UNIT_ATTENTION ||
+		     sshdr->sense_key == NOT_READY))
 			sdev->changed = 1;
 
 	} while (retries > 0 &&

Re: [regression] CD-DA delay needed after insertion

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5421 bytes --]

On Wed, 18 Jun 2008, James Bottomley wrote:
> On Tue, 2008-06-17 at 15:31 +0200, Geert Uytterhoeven wrote:
> > On Mon, 16 Jun 2008, James Bottomley wrote:
> > > On Mon, 2008-06-16 at 17:05 +0200, Geert Uytterhoeven wrote:
> > > > On Sun, 15 Jun 2008, James Bottomley wrote:
> > > > > On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> > > > > > We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> > > > > > 
> > > > > > It can easily be reproduced by:
> > > > > > 
> > > > > >   1. Inserting an audio CD
> > > > > >   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> > > > > >      stops blinking and is lit continuously:
> > > > > >       
> > > > > > 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> > > > > > 
> > > > > > On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> > > > > > insertion, or it will fail.
> > > > > > On 2.6.24 and older, it just works immediately.
> > > > > > 
> > > > > > It does not matter whether
> > > > > > http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> > > > > > is applied or not.
> > > > > > 
> > > > > > We haven't bisected it yet.
> > > > > 
> > > > > There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> > > > > so I'd bet on the previous culprits.
> > > > > 
> > > > > This time, the taxonomy looks like NOT_READY isn't being waited for
> > > > > properly.  I'd still tend to blame
> > > > > 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
> > > > 
> > > > Reverting that commit doesn't fix the problem.
> > > 
> > > That's a bit of a problem.  Particularly as:
> > > 
> > > > > suspect this to be the problem line:
> > > > > 
> > > > > 	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> > > > > 		return CDS_DISC_OK;
> > > > > 
> > > > > Previously the code would have returned CDS_NO_DISC for the not ready
> > > > > case.  The CD would have tried to close the door but (and this is the
> > > > > key) it would have waited for the door to close before trying again.
> > > > 
> > > > I tried the patch below.
> > > > 
> > > > Now the kernel spits out messages every 2 seconds:
> > > > 
> > > > | G: CDS_NO_DISC
> > > > | G: CDS_NO_DISC
> > > > 
> > > > Insert CD-DA media
> > > > 
> > > > Reading from CD using cdparanioa fails
> > > > 
> > > > | F: CDS_DISC_OK
> > > > | F: CDS_DISC_OK
> > > > | F: CDS_DISC_OK
> > > > | F: CDS_DISC_OK
> > > > | F: CDS_DISC_OK
> > > 
> > > That's in the two lines I fingered as the source of the problem ...
> > > however, if reversing the apparently bad commit (that introduces those
> > > lines) doesn't work there's something else wrong.
> > > 
> > > Could you try altering your F: case to return CDS_DRIVE_NOT_READY, but
> > > if that doesn't work, it's going to take a bit of bisection, I'm
> > > afraid ...
> > 
> > Doesn't make any difference.
> > 
> > git-bisect taught me it was introduced by
> > 
> > commit 38582a62ecd337de4212004c7d4844899dc57890
> > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Date:   Wed Feb 6 13:01:58 2008 -0600
> > 
> >     [SCSI] sr: fix test unit ready responses
> >     
> >     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
> >     the scsi_test_unit_ready() function.  Unfortunately, this has the
> >     wrong characteristic of eating NOT_READY returns which sr.c relies on
> >     for tray status.
> >     
> >     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.
> 
> OK, that basically just replaced scsi_test_unit_ready with
> sr_test_unit_ready.  The only difference I can see is that
> scsi_test_unit_ready treats NOT_READY as a media change event and the
> new code doesn't.  This seems slightly wrong:  NOT_READY can indeed mean
> I have no medium, which is obviously what it's looking for.  However, it
> can also mean there's another command in progress.  If this is some type
> of write or format, you don't necessarily want the cd treating it as a
> disk change.
> 
> However, before we get into serious debugging, could you try this patch
> (applies over the previous fix for the capacity problem)?  I think it
> should restore the old not ready == medium change behaviour.
> 
> Thanks,
> 
> James
> 
> ---
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index c82df8b..a9acbc8 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -179,7 +179,8 @@ int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
>  					      0, sshdr, SR_TIMEOUT,
>  					      retries--);
>  		if (scsi_sense_valid(sshdr) &&
> -		    sshdr->sense_key == UNIT_ATTENTION)
> +		    (sshdr->sense_key == UNIT_ATTENTION ||
> +		     sshdr->sense_key == NOT_READY))
>  			sdev->changed = 1;
>  
>  	} while (retries > 0 &&

Thanks, but unfortunately it doesn't help.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Technology and Software Centre Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: [regression] CD-DA delay needed after insertion

[James Bottomley]

On Tue, 2008-06-17 at 15:31 +0200, Geert Uytterhoeven wrote:
> On Mon, 16 Jun 2008, James Bottomley wrote:
> > On Mon, 2008-06-16 at 17:05 +0200, Geert Uytterhoeven wrote:
> > > On Sun, 15 Jun 2008, James Bottomley wrote:
> > > > On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> > > > > We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> > > > > 
> > > > > It can easily be reproduced by:
> > > > > 
> > > > >   1. Inserting an audio CD
> > > > >   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> > > > >      stops blinking and is lit continuously:
> > > > >       
> > > > > 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> > > > > 
> > > > > On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> > > > > insertion, or it will fail.
> > > > > On 2.6.24 and older, it just works immediately.
> > > > > 
> > > > > It does not matter whether
> > > > > http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> > > > > is applied or not.
> > > > > 
> > > > > We haven't bisected it yet.
> > > > 
> > > > There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> > > > so I'd bet on the previous culprits.
> > > > 
> > > > This time, the taxonomy looks like NOT_READY isn't being waited for
> > > > properly.  I'd still tend to blame
> > > > 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
> > > 
> > > Reverting that commit doesn't fix the problem.
> > 
> > That's a bit of a problem.  Particularly as:
> > 
> > > > suspect this to be the problem line:
> > > > 
> > > > 	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> > > > 		return CDS_DISC_OK;
> > > > 
> > > > Previously the code would have returned CDS_NO_DISC for the not ready
> > > > case.  The CD would have tried to close the door but (and this is the
> > > > key) it would have waited for the door to close before trying again.
> > > 
> > > I tried the patch below.
> > > 
> > > Now the kernel spits out messages every 2 seconds:
> > > 
> > > | G: CDS_NO_DISC
> > > | G: CDS_NO_DISC
> > > 
> > > Insert CD-DA media
> > > 
> > > Reading from CD using cdparanioa fails
> > > 
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > > | F: CDS_DISC_OK
> > 
> > That's in the two lines I fingered as the source of the problem ...
> > however, if reversing the apparently bad commit (that introduces those
> > lines) doesn't work there's something else wrong.
> > 
> > Could you try altering your F: case to return CDS_DRIVE_NOT_READY, but
> > if that doesn't work, it's going to take a bit of bisection, I'm
> > afraid ...
> 
> Doesn't make any difference.
> 
> git-bisect taught me it was introduced by
> 
> commit 38582a62ecd337de4212004c7d4844899dc57890
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Wed Feb 6 13:01:58 2008 -0600
> 
>     [SCSI] sr: fix test unit ready responses
>     
>     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
>     the scsi_test_unit_ready() function.  Unfortunately, this has the
>     wrong characteristic of eating NOT_READY returns which sr.c relies on
>     for tray status.
>     
>     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.

OK, I thought I had a test case for this, but when I revert this commit
on git head (and fix up the one reject which just leaves the sr_
function in place) I still produce the same behaviour.

What I'm trying is 

sg_start -i -l <cdrom>

to close the tray followed by your cdparanoia command

Could you see if reverting this commit on git head works for you (in
which case I'm not reproducing it correctly)?

Thanks,

James

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1976 bytes --]

	Hi James,

On Fri, 27 Jun 2008, James Bottomley wrote:
> > git-bisect taught me it was introduced by
> > 
> > commit 38582a62ecd337de4212004c7d4844899dc57890
> > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Date:   Wed Feb 6 13:01:58 2008 -0600
> > 
> >     [SCSI] sr: fix test unit ready responses
> >     
> >     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
> >     the scsi_test_unit_ready() function.  Unfortunately, this has the
> >     wrong characteristic of eating NOT_READY returns which sr.c relies on
> >     for tray status.
> >     
> >     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.
> 
> OK, I thought I had a test case for this, but when I revert this commit
> on git head (and fix up the one reject which just leaves the sr_
> function in place) I still produce the same behaviour.
> 
> What I'm trying is 
> 
> sg_start -i -l <cdrom>
> 
> to close the tray followed by your cdparanoia command
> 
> Could you see if reverting this commit on git head works for you (in
> which case I'm not reproducing it correctly)?

On 9bedbcb207ed9a571b239231d99c8fd4a34ae24d, the sequence

    eject; sg_start -i -l /dev/scd0; cdparanoia \
	-d /dev/scd0 -Z -q 1-1[:1] /dev/null || echo failed

fails with

    004: Unable to read table of contents header

After reverting 38582a62ecd337de4212004c7d4844899dc57890, it works.

I added the eject as the PS3 has a slot-loading drive.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Technology and Software Centre Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[James Bottomley]

On Mon, 2008-06-30 at 11:25 +0200, Geert Uytterhoeven wrote:
> 	Hi James,
> 
> On Fri, 27 Jun 2008, James Bottomley wrote:
> > > git-bisect taught me it was introduced by
> > > 
> > > commit 38582a62ecd337de4212004c7d4844899dc57890
> > > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > Date:   Wed Feb 6 13:01:58 2008 -0600
> > > 
> > >     [SCSI] sr: fix test unit ready responses
> > >     
> > >     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
> > >     the scsi_test_unit_ready() function.  Unfortunately, this has the
> > >     wrong characteristic of eating NOT_READY returns which sr.c relies on
> > >     for tray status.
> > >     
> > >     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.
> > 
> > OK, I thought I had a test case for this, but when I revert this commit
> > on git head (and fix up the one reject which just leaves the sr_
> > function in place) I still produce the same behaviour.
> > 
> > What I'm trying is 
> > 
> > sg_start -i -l <cdrom>
> > 
> > to close the tray followed by your cdparanoia command
> > 
> > Could you see if reverting this commit on git head works for you (in
> > which case I'm not reproducing it correctly)?
> 
> On 9bedbcb207ed9a571b239231d99c8fd4a34ae24d, the sequence
> 
>     eject; sg_start -i -l /dev/scd0; cdparanoia \
> 	-d /dev/scd0 -Z -q 1-1[:1] /dev/null || echo failed
> 
> fails with
> 
>     004: Unable to read table of contents header
> 
> After reverting 38582a62ecd337de4212004c7d4844899dc57890, it works.
> 
> I added the eject as the PS3 has a slot-loading drive.
> 
> With kind regards,

OK ... but this doesn't happen for me with or without this commit
reverted.  This is what I see from stracing cdparanoia:

lstat64("/dev/scd0", {st_mode=S_IFBLK|0660, st_rdev=makedev(11, 0), ...}) = 0
open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
ioctl(3, SG_IO, 0xbfc0afdc)             = -1 EINVAL (Invalid argument)
close(3)                                = 0
open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
dup(3)                                  = 4
ioctl(3, CDROMAUDIOBUFSIZ or SCSI_IOCTL_GET_IDLUN, 0xbfc0b240) = 0
ioctl(3, SCSI_IOCTL_GET_BUS_NUMBER, 0xbfc0b248) = 0
ioctl(3, SG_IO, 0xbfc0af8c)             = 0
ioctl(4, SG_EMULATED_HOST, 0xbfc0b2b8)  = 0
ioctl(3, SG_IO, 0xbfc0b07c)             = 0
ioctl(3, SG_IO, 0xbfc0b04c)             = 0
write(2, "004: Unable to read table of con"..., 45004: Unable to read table of contents header) = 45

The O_NONBLOCK tells the cdrom layer not to do drive ready processing.

The three SG_IOs are INQUIRY, MODE_SENSE(10) and READ TOC/PMA/ATIP

The latter one is returned with a check condition and sense data not
ready; medium not present - tray open.

There's no way the drive ready processing can have an effect on this
sequence unless cdparanoia either invokes it from the cdrom layer or
processes the not ready itself.  Could you strace your cdparanoia and
see what sequence it is using?

Thanks,

James

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[James Bottomley]

On Mon, 2008-06-30 at 10:51 -0500, James Bottomley wrote:
> On Mon, 2008-06-30 at 11:25 +0200, Geert Uytterhoeven wrote:
> > 	Hi James,
> > 
> > On Fri, 27 Jun 2008, James Bottomley wrote:
> > > > git-bisect taught me it was introduced by
> > > > 
> > > > commit 38582a62ecd337de4212004c7d4844899dc57890
> > > > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > Date:   Wed Feb 6 13:01:58 2008 -0600
> > > > 
> > > >     [SCSI] sr: fix test unit ready responses
> > > >     
> > > >     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
> > > >     the scsi_test_unit_ready() function.  Unfortunately, this has the
> > > >     wrong characteristic of eating NOT_READY returns which sr.c relies on
> > > >     for tray status.
> > > >     
> > > >     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.
> > > 
> > > OK, I thought I had a test case for this, but when I revert this commit
> > > on git head (and fix up the one reject which just leaves the sr_
> > > function in place) I still produce the same behaviour.
> > > 
> > > What I'm trying is 
> > > 
> > > sg_start -i -l <cdrom>
> > > 
> > > to close the tray followed by your cdparanoia command
> > > 
> > > Could you see if reverting this commit on git head works for you (in
> > > which case I'm not reproducing it correctly)?
> > 
> > On 9bedbcb207ed9a571b239231d99c8fd4a34ae24d, the sequence
> > 
> >     eject; sg_start -i -l /dev/scd0; cdparanoia \
> > 	-d /dev/scd0 -Z -q 1-1[:1] /dev/null || echo failed
> > 
> > fails with
> > 
> >     004: Unable to read table of contents header
> > 
> > After reverting 38582a62ecd337de4212004c7d4844899dc57890, it works.
> > 
> > I added the eject as the PS3 has a slot-loading drive.
> > 
> > With kind regards,
> 
> OK ... but this doesn't happen for me with or without this commit
> reverted.  This is what I see from stracing cdparanoia:
> 
> lstat64("/dev/scd0", {st_mode=S_IFBLK|0660, st_rdev=makedev(11, 0), ...}) = 0
> open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
> ioctl(3, SG_IO, 0xbfc0afdc)             = -1 EINVAL (Invalid argument)
> close(3)                                = 0
> open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
> dup(3)                                  = 4
> ioctl(3, CDROMAUDIOBUFSIZ or SCSI_IOCTL_GET_IDLUN, 0xbfc0b240) = 0
> ioctl(3, SCSI_IOCTL_GET_BUS_NUMBER, 0xbfc0b248) = 0
> ioctl(3, SG_IO, 0xbfc0af8c)             = 0
> ioctl(4, SG_EMULATED_HOST, 0xbfc0b2b8)  = 0
> ioctl(3, SG_IO, 0xbfc0b07c)             = 0
> ioctl(3, SG_IO, 0xbfc0b04c)             = 0
> write(2, "004: Unable to read table of con"..., 45004: Unable to read table of contents header) = 45
> 
> The O_NONBLOCK tells the cdrom layer not to do drive ready processing.
> 
> The three SG_IOs are INQUIRY, MODE_SENSE(10) and READ TOC/PMA/ATIP
> 
> The latter one is returned with a check condition and sense data not
> ready; medium not present - tray open.
> 
> There's no way the drive ready processing can have an effect on this
> sequence unless cdparanoia either invokes it from the cdrom layer or
> processes the not ready itself.  Could you strace your cdparanoia and
> see what sequence it is using?

OK ... I think I finally found the problem.  Our DVD drives obviously
have a slight difference in the way they handle tray close.  Mine still
returns tray open for a while after the close operation has been
initiated.  To see the behaviour you need it to return not ready; in
process of becoming ready.  The zero return from scsi_test_unit_ready()
actually causes sr_media_changed() incorrectly to return zero.  But,
before it does, it tries to update the CD information and capacity.
This is where the delay occurs ... as long as the drive reports in
process of becoming ready, sr_cd_check() will wait 2s and retry until it
becomes ready and it can get the CD information.

The problem is I don't think the new behaviour is a regression.
cdparanoia requested O_NONBLOCK ... it's a bit counter to this if we
wait for the drive to become ready.  cdparanoia doesn't care that we get
the correct CD parameters on the open since it's using SG_IO to
manipulate the device.  So, I think the bug is actually in cdparanoia.
If it requests O_NONBLOCK, it's asking for full status immediately and
is supposed to be able to cope with the returns (including knowing to
retry the NOT_READY ones).

James

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5886 bytes --]

	Hi James, Monty,

On Tue, 1 Jul 2008, James Bottomley wrote:
> On Mon, 2008-06-30 at 10:51 -0500, James Bottomley wrote:
> > On Mon, 2008-06-30 at 11:25 +0200, Geert Uytterhoeven wrote:
> > > On Fri, 27 Jun 2008, James Bottomley wrote:
> > > > > git-bisect taught me it was introduced by
> > > > > 
> > > > > commit 38582a62ecd337de4212004c7d4844899dc57890
> > > > > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > Date:   Wed Feb 6 13:01:58 2008 -0600
> > > > > 
> > > > >     [SCSI] sr: fix test unit ready responses
> > > > >     
> > > > >     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
> > > > >     the scsi_test_unit_ready() function.  Unfortunately, this has the
> > > > >     wrong characteristic of eating NOT_READY returns which sr.c relies on
> > > > >     for tray status.
> > > > >     
> > > > >     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.
> > > > 
> > > > OK, I thought I had a test case for this, but when I revert this commit
> > > > on git head (and fix up the one reject which just leaves the sr_
> > > > function in place) I still produce the same behaviour.
> > > > 
> > > > What I'm trying is 
> > > > 
> > > > sg_start -i -l <cdrom>
> > > > 
> > > > to close the tray followed by your cdparanoia command
> > > > 
> > > > Could you see if reverting this commit on git head works for you (in
> > > > which case I'm not reproducing it correctly)?
> > > 
> > > On 9bedbcb207ed9a571b239231d99c8fd4a34ae24d, the sequence
> > > 
> > >     eject; sg_start -i -l /dev/scd0; cdparanoia \
> > > 	-d /dev/scd0 -Z -q 1-1[:1] /dev/null || echo failed
> > > 
> > > fails with
> > > 
> > >     004: Unable to read table of contents header
> > > 
> > > After reverting 38582a62ecd337de4212004c7d4844899dc57890, it works.
> > > 
> > > I added the eject as the PS3 has a slot-loading drive.
> > > 
> > > With kind regards,
> > 
> > OK ... but this doesn't happen for me with or without this commit
> > reverted.  This is what I see from stracing cdparanoia:
> > 
> > lstat64("/dev/scd0", {st_mode=S_IFBLK|0660, st_rdev=makedev(11, 0), ...}) = 0
> > open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
> > ioctl(3, SG_IO, 0xbfc0afdc)             = -1 EINVAL (Invalid argument)
> > close(3)                                = 0
> > open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
> > dup(3)                                  = 4
> > ioctl(3, CDROMAUDIOBUFSIZ or SCSI_IOCTL_GET_IDLUN, 0xbfc0b240) = 0
> > ioctl(3, SCSI_IOCTL_GET_BUS_NUMBER, 0xbfc0b248) = 0
> > ioctl(3, SG_IO, 0xbfc0af8c)             = 0
> > ioctl(4, SG_EMULATED_HOST, 0xbfc0b2b8)  = 0
> > ioctl(3, SG_IO, 0xbfc0b07c)             = 0
> > ioctl(3, SG_IO, 0xbfc0b04c)             = 0
> > write(2, "004: Unable to read table of con"..., 45004: Unable to read table of contents header) = 45
> > 
> > The O_NONBLOCK tells the cdrom layer not to do drive ready processing.
> > 
> > The three SG_IOs are INQUIRY, MODE_SENSE(10) and READ TOC/PMA/ATIP
> > 
> > The latter one is returned with a check condition and sense data not
> > ready; medium not present - tray open.
> > 
> > There's no way the drive ready processing can have an effect on this
> > sequence unless cdparanoia either invokes it from the cdrom layer or
> > processes the not ready itself.  Could you strace your cdparanoia and
> > see what sequence it is using?
> 
> OK ... I think I finally found the problem.  Our DVD drives obviously
> have a slight difference in the way they handle tray close.  Mine still
> returns tray open for a while after the close operation has been
> initiated.  To see the behaviour you need it to return not ready; in
> process of becoming ready.  The zero return from scsi_test_unit_ready()
> actually causes sr_media_changed() incorrectly to return zero.  But,
> before it does, it tries to update the CD information and capacity.
> This is where the delay occurs ... as long as the drive reports in
> process of becoming ready, sr_cd_check() will wait 2s and retry until it
> becomes ready and it can get the CD information.
> 
> The problem is I don't think the new behaviour is a regression.
> cdparanoia requested O_NONBLOCK ... it's a bit counter to this if we
> wait for the drive to become ready.  cdparanoia doesn't care that we get
> the correct CD parameters on the open since it's using SG_IO to
> manipulate the device.  So, I think the bug is actually in cdparanoia.
> If it requests O_NONBLOCK, it's asking for full status immediately and
> is supposed to be able to cope with the returns (including knowing to
> retry the NOT_READY ones).

I confirmed (using strace) that my cdparanoia also opens /dev/scd0 with
O_NONBLOCK.

After applying the patch below to cdparanoia, the problem goes away.

--- cdparanoia-3.10.0+debian/interface/scan_devices.c.orig	2008-05-16 06:25:24.000000000 +0200
+++ cdparanoia-3.10.0+debian/interface/scan_devices.c	2008-09-17 16:36:14.000000000 +0200
@@ -608,7 +608,7 @@
 
   if(specialized_device) {
     if(use_sgio)
-      i_fd=open(specialized_device,O_RDWR|O_NONBLOCK);
+      i_fd=open(specialized_device,O_RDONLY);
     else
       i_fd=open(specialized_device,O_RDONLY|O_NONBLOCK);
   }

Note that I had to change `O_RDWR' to `O_RDONLY', else you get the error -EROFS
(read only file system) when opening /dev/scd0 (music CDs are read-only).

As this patch actually modifies libcdparanoia, and not the plain cdparanoia
executable, it may have side effects on other applications...

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Monty Montgomery]

On Wed, Sep 17, 2008 at 11:05 AM, Geert Uytterhoeven
<Geert.Uytterhoeven@sonycom.com> wrote:

>> The problem is I don't think the new behaviour is a regression.
>> cdparanoia requested O_NONBLOCK ... it's a bit counter to this if we
>> wait for the drive to become ready.

O_NONBLOCK has nothing to do with the drive being ready.  It means not
to wait to open a device locked or in use exclusively by another
process. It does not affect command processing (at least not the way
cdparanoia is doing it).  If a drive isn't ready, it returns a sense
key of 'not ready' and the SGIO layer doesn't filter that in any way.
SGIO itself never waits or takes any special action on 'not ready', it
merely passes through the comand termination status.

Note that 'not ready' is not the same as 'no medium'.  One means the
drive is digesting a new disc, the other means there is no dics (or
the tray is open).  Some drives do in fact return 'empty' until a new
disc is completely loaded after tray close.  These drives are in
violation of spec (but the alternative is to ignore the empty status
and retry forever, which means there's no recovering from ejecting a
disc mid-operation, which was also reported as a bug).  On the other
hand, I've never come across any ATAPI drive that 100% faithfully
implements the command spec.  All vendors take *substantial*
liberties.

> I confirmed (using strace) that my cdparanoia also opens /dev/scd0 with
> O_NONBLOCK.
>
> After applying the patch below to cdparanoia, the problem goes away.
>
> --- cdparanoia-3.10.0+debian/interface/scan_devices.c.orig      2008-05-16 06:25:24.000000000 +0200
> +++ cdparanoia-3.10.0+debian/interface/scan_devices.c   2008-09-17 16:36:14.000000000 +0200
> @@ -608,7 +608,7 @@
>
>   if(specialized_device) {
>     if(use_sgio)
> -      i_fd=open(specialized_device,O_RDWR|O_NONBLOCK);
> +      i_fd=open(specialized_device,O_RDONLY);

This change means you can only run one cdparanoia per computer as the
device autoscan code will now block on an in-use drive.
Also, you're a few releases behind.  10.0 was misreporting drive
status due to a stupid bug; god only knows what happened in SGIO as
the error code never got passed up.  10.2 corrects the problem (as
well as a few other major bugs).  Current SVN will be 10.3 in a few
more days.

If the same thing is still happening with 10.2 and the same patch
still fixes it, then I definitely have some head scratching [and
kernel perusal] to do.

Also, the SCSI and block layers tend to not be entirely in sync
behaviorwise despite exposing what appears to be a uniform SGIO
interface (it's actually two completely seperate interfaces that try
to look identical).  I'd not rule out kernel bugs, it wouldn't be the
first (or only the tenth) that cdparanoia has exposed :-(

>     else
>       i_fd=open(specialized_device,O_RDONLY|O_NONBLOCK);
>   }
>
> Note that I had to change `O_RDWR' to `O_RDONLY', else you get the error -EROFS
> (read only file system) when opening /dev/scd0 (music CDs are read-only).

It has nothing to do with the CD being read only.  Device permissions
are overloaded in SGIO to act as a two-tier permissions filter to
seperate  'harmless' and 'potentially harmful' commands. You have to
have write permission on the device to do things like MODE SET, which
some drives require (eg, Plextor) or to use the short-DMA detection
hack to cover for low-level device drivers that don't bother to
implement the residue flag.  You can blame Jens for this one.

Monty

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3739 bytes --]

	Hi Monty,

On Tue, 30 Sep 2008, Monty Montgomery wrote:
> On Wed, Sep 17, 2008 at 11:05 AM, Geert Uytterhoeven
> <Geert.Uytterhoeven@sonycom.com> wrote:
> >> The problem is I don't think the new behaviour is a regression.
> >> cdparanoia requested O_NONBLOCK ... it's a bit counter to this if we
> >> wait for the drive to become ready.
> 
> O_NONBLOCK has nothing to do with the drive being ready.  It means not
> to wait to open a device locked or in use exclusively by another
> process. It does not affect command processing (at least not the way
> cdparanoia is doing it).  If a drive isn't ready, it returns a sense
> key of 'not ready' and the SGIO layer doesn't filter that in any way.
> SGIO itself never waits or takes any special action on 'not ready', it
> merely passes through the comand termination status.
> 
> Note that 'not ready' is not the same as 'no medium'.  One means the
> drive is digesting a new disc, the other means there is no dics (or
> the tray is open).  Some drives do in fact return 'empty' until a new
> disc is completely loaded after tray close.  These drives are in
> violation of spec (but the alternative is to ignore the empty status
> and retry forever, which means there's no recovering from ejecting a
> disc mid-operation, which was also reported as a bug).  On the other
> hand, I've never come across any ATAPI drive that 100% faithfully
> implements the command spec.  All vendors take *substantial*
> liberties.
> 
> > I confirmed (using strace) that my cdparanoia also opens /dev/scd0 with
> > O_NONBLOCK.
> >
> > After applying the patch below to cdparanoia, the problem goes away.
> >
> > --- cdparanoia-3.10.0+debian/interface/scan_devices.c.orig      2008-05-16 06:25:24.000000000 +0200
> > +++ cdparanoia-3.10.0+debian/interface/scan_devices.c   2008-09-17 16:36:14.000000000 +0200
> > @@ -608,7 +608,7 @@
> >
> >   if(specialized_device) {
> >     if(use_sgio)
> > -      i_fd=open(specialized_device,O_RDWR|O_NONBLOCK);
> > +      i_fd=open(specialized_device,O_RDONLY);
> 
> This change means you can only run one cdparanoia per computer as the
> device autoscan code will now block on an in-use drive.
> Also, you're a few releases behind.  10.0 was misreporting drive
> status due to a stupid bug; god only knows what happened in SGIO as
> the error code never got passed up.  10.2 corrects the problem (as
> well as a few other major bugs).  Current SVN will be 10.3 in a few
> more days.

So I tried the version in Debian unstable...

> If the same thing is still happening with 10.2 and the same patch

The same thing happens with cdparanoia-3.10.2+debian.

> still fixes it, then I definitely have some head scratching [and
> kernel perusal] to do.

But the patch above doesn't help. Now cdparanoia crashes with a segmentation
fault:

| 0x0ffd5730 in cdda_read_timed () from /usr/lib/libcdda_interface.so.0
| (gdb) bt
| #0  0x0ffd5730 in cdda_read_timed () from /usr/lib/libcdda_interface.so.0
| #1  0x0ffa7624 in i_read_c_block () from /usr/lib/libcdda_paranoia.so.0
| #2  0x0ffa9114 in paranoia_read_limited () from /usr/lib/libcdda_paranoia.so.0
| #3  0x10004da0 in ?? ()
| #4  0x0fd46704 in ?? () from /lib/libc.so.6
| #5  0x0fd468c0 in __libc_start_main () from /lib/libc.so.6
| #6  0x00000000 in ?? ()

due to the `buffer' parameter being NULL.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Monty Montgomery]

> | 0x0ffd5730 in cdda_read_timed () from /usr/lib/libcdda_interface.so.0
> | (gdb) bt
> | #0  0x0ffd5730 in cdda_read_timed () from /usr/lib/libcdda_interface.so.0
> | #1  0x0ffa7624 in i_read_c_block () from /usr/lib/libcdda_paranoia.so.0
> | #2  0x0ffa9114 in paranoia_read_limited () from /usr/lib/libcdda_paranoia.so.0
> | #3  0x10004da0 in ?? ()
> | #4  0x0fd46704 in ?? () from /lib/libc.so.6
> | #5  0x0fd468c0 in __libc_start_main () from /lib/libc.so.6
> | #6  0x00000000 in ?? ()
>
> due to the `buffer' parameter being NULL.

Oh, good catch.  Platform/drive combination that hadn't been tripped
here.  Fixing in SVN now.

(you're on bigendian or using an old bigendian SCSI drive?)

Monty

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Monty Montgomery]

Fixed in SVN; pull a copy and try again.

[Mein Gott, that bug was just embarrassing]

Monty

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1295 bytes --]

On Tue, 30 Sep 2008, Monty Montgomery wrote:
> Fixed in SVN; pull a copy and try again.
> 
> [Mein Gott, that bug was just embarrassing]

With latest cdparanoia git-^H^H^H^Hsvn, the problem still happens.
After applying the patch below, there's no more crash thanks to your last
commit, and It Just Works(tm):

    eject; sg_start -i -l /dev/scd0; cdparanoia -d /dev/scd0 -Z -q 1-1[:1] /dev/null || echo failed

diff --git a/interface/scan_devices.c b/interface/scan_devices.c
index bfe5403..df08906 100644
--- a/interface/scan_devices.c
+++ b/interface/scan_devices.c
@@ -614,7 +614,7 @@ cdrom_drive *cdda_identify_scsi(const char *generic_device,
 
   if(specialized_device) {
     if(use_sgio)
-      i_fd=open(specialized_device,O_RDWR|O_NONBLOCK);
+      i_fd=open(specialized_device,O_RDONLY);
     else
       i_fd=open(specialized_device,O_RDONLY|O_NONBLOCK);
   }

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Monty Montgomery]

> With latest cdparanoia git-^H^H^H^Hsvn, the problem still happens.
> After applying the patch below, there's no more crash thanks to your last
> commit, and It Just Works(tm):

Then I will be doing some headscratching over a copy of the kernel.
Oh--  what *exact* kernel are you using?  non-x86 trees don't always
sync often with the 'official' mainline and I want to be sure I'm
reading the correct thing.  I was a longtime PPC user until about two
years ago, and I'm used to there being several substantially different
PPC kernel trees [just making sure].

OTOH, if the kernel isn't even servicing the device as a block device
until the drive returns a ready status and O_NONBLOCK thwarts waiting
for that.... sigh.  Nothing like a passthrough interface that still
isn't a passthrough interface after fifteen years of dicking around.

I'm not willing to cripple cdparanoia on more-than-one-cdrom systems
because of an edge case due to yet another ill-considered block layer
'feature'.  For one thing *I* need to be able to scan devices without
blocking on my own multiple drive systems.  If there's a workaround to
get both, I will find it.

But it's too early to say for sure.  I'll check again.

(May I ask-- why the hard starting/stopping of the interface?  Power saving?]

Monty

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2073 bytes --]

On Tue, 30 Sep 2008, Monty Montgomery wrote:
> > With latest cdparanoia git-^H^H^H^Hsvn, the problem still happens.
> > After applying the patch below, there's no more crash thanks to your last
> > commit, and It Just Works(tm):
> 
> Then I will be doing some headscratching over a copy of the kernel.
> Oh--  what *exact* kernel are you using?  non-x86 trees don't always
> sync often with the 'official' mainline and I want to be sure I'm
> reading the correct thing.  I was a longtime PPC user until about two
> years ago, and I'm used to there being several substantially different
> PPC kernel trees [just making sure].

It happens on anything after 2.6.24 (cfr.
http://bugzilla.kernel.org/show_bug.cgi?id=10974).
E.g. mainline 2.6.27-rc8.

> OTOH, if the kernel isn't even servicing the device as a block device
> until the drive returns a ready status and O_NONBLOCK thwarts waiting
> for that.... sigh.  Nothing like a passthrough interface that still
> isn't a passthrough interface after fifteen years of dicking around.
> 
> I'm not willing to cripple cdparanoia on more-than-one-cdrom systems
> because of an edge case due to yet another ill-considered block layer
> 'feature'.  For one thing *I* need to be able to scan devices without
> blocking on my own multiple drive systems.  If there's a workaround to
> get both, I will find it.
> 
> But it's too early to say for sure.  I'll check again.

The difficult part is who's to blame: Linux commit
38582a62ecd337de4212004c7d4844899dc57890 or cdparanoia?

> (May I ask-- why the hard starting/stopping of the interface?  Power saving?]

Which hard starting/stopping do you mean?

Thanks!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Monty Montgomery]

On Tue, Sep 30, 2008 at 11:21 AM, Geert Uytterhoeven
<Geert.Uytterhoeven@sonycom.com> wrote:

>> (May I ask-- why the hard starting/stopping of the interface?  Power saving?]
>
> Which hard starting/stopping do you mean?

why the calls to sg_start?

Monty

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Matthew Wilcox]

On Tue, Sep 30, 2008 at 01:32:15PM -0400, Monty Montgomery wrote:
> On Tue, Sep 30, 2008 at 11:21 AM, Geert Uytterhoeven
> <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> >> (May I ask-- why the hard starting/stopping of the interface?  Power saving?]
> why the calls to sg_start?

In this case, sg means scatter/gather, and it's a purely internal
interface.  It does not start/stop the drive, nor the scsi interface.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Matthew Wilcox]

On Tue, Sep 30, 2008 at 11:41:51AM -0600, Matthew Wilcox wrote:
> On Tue, Sep 30, 2008 at 01:32:15PM -0400, Monty Montgomery wrote:
> > On Tue, Sep 30, 2008 at 11:21 AM, Geert Uytterhoeven
> > <Geert.Uytterhoeven@sonycom.com> wrote:
> > 
> > >> (May I ask-- why the hard starting/stopping of the interface?  Power saving?]
> > why the calls to sg_start?
> 
> In this case, sg means scatter/gather, and it's a purely internal
> interface.  It does not start/stop the drive, nor the scsi interface.

That'll teach me to comment before reading the entire thread.  Please
ignore this mistaken explanation; I thought you were commenting on a
patch, not on the commands used at the command line.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Monty Montgomery]

> It happens on anything after 2.6.24 (cfr.
> http://bugzilla.kernel.org/show_bug.cgi?id=10974).
> E.g. mainline 2.6.27-rc8.

Ah.

Geert, I'm sorry this has come up and thanks for bringing it to my attention.

I'm relying on 15 year old behavior.  It has always been the case that
the packet pass-through interfaces treat O_NONBLOCK as 'don't wait for
commands' (ie, classic async communication or, in the case of SGIO,
don't block on mandatory locks) not 'change how you mediate the
communication'.  The whole point of the passthrough interface is
'please don't fuck with the packet stream.  My userspace application
knows better than the kernel in this case'.  If the kernel has
suddenly changed that, it is responsible for the loss of
functionality, not cdparanoia.

It is also the case that the kernel randomly interspersing its own
packets, commands and setup would be yanking setup state out from
under cdparanoia.  What am I supposed to do if I set density and
completely change the data mode of a cdrom drive (persistent across
disc changes), or change the block descriptor caching extents, only to
have the kernel decide to change them back without warning? If you
eject media with one of these new kernels, do I still get any
meaningful sense state, or is the sense key and all subsequent
commands merely intercepted and refused?  What happens on a timeout?
A media error?  A bus reset?  Does any 'CHECK CONDITION' status
completely yank the rug out from under the passthrough?

The kernel must not interfere with the pass-through or cdparanoia is
castrated. Or... I'm back to the era of enacting workarounds based on
kernel dot-version.  Regardless, this is a change to a longstanding
behavior.  Is it on purpose, unintentional or 'intentional but without
realizing it was actually important'?  Would be interesting to know.

> In this case, sg means scatter/gather, and it's a purely internal
> interface.  It does not start/stop the drive, nor the scsi interface.

Ah, I confused it with a much older utility that would up/down an SG
device entirely.

Monty

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1220 bytes --]

On Tue, 30 Sep 2008, Monty Montgomery wrote:
> > It happens on anything after 2.6.24 (cfr.
> > http://bugzilla.kernel.org/show_bug.cgi?id=10974).
> > E.g. mainline 2.6.27-rc8.
> 
> Ah.
> 
> Geert, I'm sorry this has come up and thanks for bringing it to my attention.

The bugzilla link has been in the subjects of the emails all the time ;-)

> > In this case, sg means scatter/gather, and it's a purely internal
> > interface.  It does not start/stop the drive, nor the scsi interface.
> 
> Ah, I confused it with a much older utility that would up/down an SG
> device entirely.

It is the command line utility.
James suggested to use it to load the CD.

Before that, my test sequence was less deterministic and not fully automated
(insert CD, wait until the blue LED is lit, launch cdparanoia).

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1298 bytes --]

	Hi Monty,

On Tue, 30 Sep 2008, Monty Montgomery wrote:
> > | 0x0ffd5730 in cdda_read_timed () from /usr/lib/libcdda_interface.so.0
> > | (gdb) bt
> > | #0  0x0ffd5730 in cdda_read_timed () from /usr/lib/libcdda_interface.so.0
> > | #1  0x0ffa7624 in i_read_c_block () from /usr/lib/libcdda_paranoia.so.0
> > | #2  0x0ffa9114 in paranoia_read_limited () from /usr/lib/libcdda_paranoia.so.0
> > | #3  0x10004da0 in ?? ()
> > | #4  0x0fd46704 in ?? () from /lib/libc.so.6
> > | #5  0x0fd468c0 in __libc_start_main () from /lib/libc.so.6
> > | #6  0x00000000 in ?? ()
> >
> > due to the `buffer' parameter being NULL.
> 
> Oh, good catch.  Platform/drive combination that hadn't been tripped
> here.  Fixing in SVN now.

OK, I'll take a look...

> (you're on bigendian or using an old bigendian SCSI drive?)

Yep. This is on PS3, which is ppc64 (albeit most userland is ppc32).

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [regression] CD-DA delay needed after insertion (http://bugzilla.kernel.org/show_bug.cgi?id=10974)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3668 bytes --]

	Hi James,

On Mon, 30 Jun 2008, James Bottomley wrote:
> On Mon, 2008-06-30 at 11:25 +0200, Geert Uytterhoeven wrote:
> > On Fri, 27 Jun 2008, James Bottomley wrote:
> > > > git-bisect taught me it was introduced by
> > > > 
> > > > commit 38582a62ecd337de4212004c7d4844899dc57890
> > > > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > Date:   Wed Feb 6 13:01:58 2008 -0600
> > > > 
> > > >     [SCSI] sr: fix test unit ready responses
> > > >     
> > > >     Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 updated sr.c to use
> > > >     the scsi_test_unit_ready() function.  Unfortunately, this has the
> > > >     wrong characteristic of eating NOT_READY returns which sr.c relies on
> > > >     for tray status.
> > > >     
> > > >     Fix by rolling an internal sr_test_unit_ready() that doesn't do this.
> > > 
> > > OK, I thought I had a test case for this, but when I revert this commit
> > > on git head (and fix up the one reject which just leaves the sr_
> > > function in place) I still produce the same behaviour.
> > > 
> > > What I'm trying is 
> > > 
> > > sg_start -i -l <cdrom>
> > > 
> > > to close the tray followed by your cdparanoia command
> > > 
> > > Could you see if reverting this commit on git head works for you (in
> > > which case I'm not reproducing it correctly)?
> > 
> > On 9bedbcb207ed9a571b239231d99c8fd4a34ae24d, the sequence
> > 
> >     eject; sg_start -i -l /dev/scd0; cdparanoia \
> > 	-d /dev/scd0 -Z -q 1-1[:1] /dev/null || echo failed
> > 
> > fails with
> > 
> >     004: Unable to read table of contents header
> > 
> > After reverting 38582a62ecd337de4212004c7d4844899dc57890, it works.
> > 
> > I added the eject as the PS3 has a slot-loading drive.
> > 
> > With kind regards,
> 
> OK ... but this doesn't happen for me with or without this commit
> reverted.  This is what I see from stracing cdparanoia:
> 
> lstat64("/dev/scd0", {st_mode=S_IFBLK|0660, st_rdev=makedev(11, 0), ...}) = 0
> open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
> ioctl(3, SG_IO, 0xbfc0afdc)             = -1 EINVAL (Invalid argument)
> close(3)                                = 0
> open("/dev/scd0", O_RDWR|O_NONBLOCK)    = 3
> dup(3)                                  = 4
> ioctl(3, CDROMAUDIOBUFSIZ or SCSI_IOCTL_GET_IDLUN, 0xbfc0b240) = 0
> ioctl(3, SCSI_IOCTL_GET_BUS_NUMBER, 0xbfc0b248) = 0
> ioctl(3, SG_IO, 0xbfc0af8c)             = 0
> ioctl(4, SG_EMULATED_HOST, 0xbfc0b2b8)  = 0
> ioctl(3, SG_IO, 0xbfc0b07c)             = 0
> ioctl(3, SG_IO, 0xbfc0b04c)             = 0
> write(2, "004: Unable to read table of con"..., 45004: Unable to read table of contents header) = 45
> 
> The O_NONBLOCK tells the cdrom layer not to do drive ready processing.
> 
> The three SG_IOs are INQUIRY, MODE_SENSE(10) and READ TOC/PMA/ATIP
> 
> The latter one is returned with a check condition and sense data not
> ready; medium not present - tray open.
> 
> There's no way the drive ready processing can have an effect on this
> sequence unless cdparanoia either invokes it from the cdrom layer or
> processes the not ready itself.  Could you strace your cdparanoia and
> see what sequence it is using?

My cdparanioa is using the exact same sequence.

Sorry it took that long to get back to you (holidays and OLS).

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@eu.sony.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: [regression] CD-DA delay needed after insertion

[Chuck Ebbert]

James Bottomley wrote:
> On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
>> We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
>>
>> It can easily be reproduced by:
>>
>>   1. Inserting an audio CD
>>   2. Running the following command as soon as the blue CD/DVD/BD drive LED
>>      stops blinking and is lit continuously:
>>       
>> 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
>>
>> On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
>> insertion, or it will fail.
>> On 2.6.24 and older, it just works immediately.
>>
>> It does not matter whether
>> http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
>> is applied or not.
>>
>> We haven't bisected it yet.
> 
> There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> so I'd bet on the previous culprits.
> 
> This time, the taxonomy looks like NOT_READY isn't being waited for
> properly.  I'd still tend to blame
> 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
> suspect this to be the problem line:
> 

Looking at the last part of that commit, what code path could ever lead 
to reaching that last return statement and returning 
CDS_DRIVE_NOT_READY? Maybe I'm just being dense but it looks unreachable 
to me:

+	/*
+	 * 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 (scsi_sense_valid(&sshdr) &&
+	    /* 0x3a is medium not present */
+	    sshdr.asc == 0x3a)
+		return CDS_NO_DISC;
+	else
+		return CDS_TRAY_OPEN;
+
+	return CDS_DRIVE_NOT_READY;
  }

Re: [regression] CD-DA delay needed after insertion

[James Bottomley]

On Tue, 2008-06-17 at 18:56 -0400, Chuck Ebbert wrote:
> James Bottomley wrote:
> > On Fri, 2008-06-13 at 13:57 +0200, Geert Uytterhoeven wrote:
> >> We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
> >>
> >> It can easily be reproduced by:
> >>
> >>   1. Inserting an audio CD
> >>   2. Running the following command as soon as the blue CD/DVD/BD drive LED
> >>      stops blinking and is lit continuously:
> >>       
> >> 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
> >>
> >> On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> >> insertion, or it will fail.
> >> On 2.6.24 and older, it just works immediately.
> >>
> >> It does not matter whether
> >> http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> >> is applied or not.
> >>
> >> We haven't bisected it yet.
> > 
> > There aren't that many commits affecting sr between 2.6.24 and 2.6.25,
> > so I'd bet on the previous culprits.
> > 
> > This time, the taxonomy looks like NOT_READY isn't being waited for
> > properly.  I'd still tend to blame
> > 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 it's just that this time I
> > suspect this to be the problem line:
> > 
> 
> Looking at the last part of that commit, what code path could ever lead 
> to reaching that last return statement and returning 
> CDS_DRIVE_NOT_READY? Maybe I'm just being dense but it looks unreachable 
> to me:

It is ... unfortunately, it's also been verified not to be the problem
(that's what I thought at first too) ... it looks like drivers/cdrom has
no real use for CDS_DRIVE_NOT_READY.

James

Re: [regression] CD-DA delay needed after insertion

[Pat Read]

> We've found another regression in 2.6.25 w.r.t. CD media change on PS3.
>
> It can easily be reproduced by:
>
>   1. Inserting an audio CD
>   2. Running the following command as soon as the blue CD/DVD/BD drive LED
>      stops blinking and is lit continuously:
>
> 	 cdparanoia -Z -q 1-1[:1] /dev/null || echo failed
>
> On 2.6.25 (and current mainline), you have to wait ca. 10 seconds after
> insertion, or it will fail.
> On 2.6.24 and older, it just works immediately.
>
> It does not matter whether
> http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fjejb%2Fscsi-rc-fixes-2.6.git;a=commitdiff_plain;h=d1daeabf0da5bfa1943272ce508e2ba785730bf0
> is applied or not.
>
> We haven't bisected it yet.
>
> With kind regards,
>
> Geert Uytterhoeven
> Software Architect
>
> Sony Techsoft Centre
> The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
>
> Phone:    +32 (0)2 700 8453
> Fax:      +32 (0)2 700 8622
> E-mail:   Geert.Uytterhoeven@sonycom.com
> Internet: http://www.sony-europe.com/
>
> Sony Technology and Software Centre Europe
> A division of Sony Service Centre (Europe) N.V.
> Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
> VAT BE 0413.825.160 · RPR Brussels
> Fortis 293-0376800-10 GEBA-BE-BB
> ---584349381-1822578177-1213358241=:2141--

Curious, might someone look at
http://bugzilla.kernel.org/show_bug.cgi?id=10901 and tell me if this
is the same issue as the above regression?

I'm currently looking at the sr_test_unit_ready() function in
drivers/scsi/sr.c (as this is new to 2.6.25), although Tejun Heo seems
skeptical that the TUR functions are the source of this issue (based
on his latest comment in Bugzilla tracker).  Thanks also to Tejun for
pointing me to this thread.

Thanks,
        Patrick A. Read