ide-cd question

ide-cd question

[Stuart_Hayes]

Why does ide-cd use "DRIVER(drive)->end_request()" instead of
"cdrom_end_request()" in the function ide_cdrom_error when it wants to
end a request that's exceeded ERROR_MAX errors?  (See code below.)

With certain requests (GET_CONFIGURATION (0x46), in particular),
DRIVER(drive)->end_request() is not actually ending the request, because
rq->bio is not NULL--but it also is not clearing rq->errors, so this
request ends up getting retried forever, but no more resets are
attempted because rq->errors has exceeded ERROR_MAX.

Cdrom_end_request(), on the other hand, WILL end the request.

Thanks,
Stuart


	if (stat & BUSY_STAT || ((stat & WRERR_STAT) && !drive->nowerr))
{
		/* other bits are useless when BUSY */
		rq->errors |= ERROR_RESET;
	} else {
		/* add decoding error stuff */
	}
	if (HWIF(drive)->INB(IDE_STATUS_REG) & (BUSY_STAT|DRQ_STAT))
		/* force an abort */
		HWIF(drive)->OUTB(WIN_IDLEIMMEDIATE,IDE_COMMAND_REG);
	if (rq->errors >= ERROR_MAX) {
		DRIVER(drive)->end_request(drive, 0, 0);  <--- should
this be cdrom_end_requeset()?
	} else {
		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
			++rq->errors;
			return ide_do_reset(drive);
		}
		++rq->errors;
	}

Re: ide-cd question

[Bartlomiej Zolnierkiewicz]

On Thu, 3 Feb 2005 14:34:42 -0600, Stuart_Hayes@dell.com
<Stuart_Hayes@dell.com> wrote:
> 
> Why does ide-cd use "DRIVER(drive)->end_request()" instead of
> "cdrom_end_request()" in the function ide_cdrom_error when it wants to
> end a request that's exceeded ERROR_MAX errors?  (See code below.)

Please always give the kernel version you are referring to.
ide_cdrom_error() is gone in 2.6.11-rc2.

If there is a bug, it is in ide_cdrom_driver declaration.
.end_request is not set to ide_cdrom_error.

> With certain requests (GET_CONFIGURATION (0x46), in particular),
> DRIVER(drive)->end_request() is not actually ending the request, because
> rq->bio is not NULL--but it also is not clearing rq->errors, so this
> request ends up getting retried forever, but no more resets are
> attempted because rq->errors has exceeded ERROR_MAX.

I don't get this, maybe I'm looking at the wrong kernel version?

Bartlomiej

RE: ide-cd question

[Stuart_Hayes]

Bartlomiej Zolnierkiewicz wrote:
> On Thu, 3 Feb 2005 14:34:42 -0600, Stuart_Hayes@dell.com
> <Stuart_Hayes@dell.com> wrote: 
>> 
>> Why does ide-cd use "DRIVER(drive)->end_request()" instead of
>> "cdrom_end_request()" in the function ide_cdrom_error when it wants
>> to end a request that's exceeded ERROR_MAX errors?  (See code below.)
> 
> Please always give the kernel version you are referring to.
> ide_cdrom_error() is gone in 2.6.11-rc2.
> 

I was looking at 2.6.10, sorry about that.  I'll have a look at
2.6.11-rc2.

> If there is a bug, it is in ide_cdrom_driver declaration.
> .end_request is not set to ide_cdrom_error.
> 

I wondered about that.  I'll give that a try.

>> With certain requests (GET_CONFIGURATION (0x46), in particular),
>> DRIVER(drive)->end_request() is not actually ending the request,
>> because rq->bio is not NULL--but it also is not clearing rq->errors,
>> so this request ends up getting retried forever, but no more resets
>> are attempted because rq->errors has exceeded ERROR_MAX.
> 
> I don't get this, maybe I'm looking at the wrong kernel version?
> 
> Bartlomiej

I should have explained this better.  DRIVER(drive)->end_request() is
actually calling default_end_request(), which calls ide_end_reuqest(),
which ends up calling __ide_end_request().  And that will only end
the request if end_that_request_first() (in ll_rw_blk.c) returns 0.
However, end_that_request_first() returns 1 because rq->bio is not
NULL (in the case of GPCMD_GET_CONFIGURATION, anyway).

So, the request isn't ended, but neither is rq->errors cleared.

Anyway, I'll have a look at 2.6.11-rc2.  Thanks for the response!

RE: ide-cd question

[Stuart_Hayes]

>> If there is a bug, it is in ide_cdrom_driver declaration.
>> .end_request is not set to ide_cdrom_error.
>> 
> 
> I wondered about that.  I'll give that a try.
> 

OK, 2.6.11-rc3 has the same issue.  The function ide_atapi_error() is
calling
drive->driver->end_request() when rq->errors exceeds ERROR_MAX, but, for
a
CD-ROM, this is calling the default end_request function.  The default
end
request function won't end the request in certain cases as described
below,
and they can get stuck being retried forever with no more resets
attempted.

It works great if I apply this patch.  I don't know if there's a reason
that
the CD-ROM driver has an end_request function but doesn't put it in the
driver
struct, or if that was an oversight, but I am assuming it was an
oversight.

I'd recommend applying this patch if nobody sees any problems with it.


--- ide-cd.c.orig	2005-02-07 10:56:42.000000000 -0500
+++ ide-cd.c	2005-02-07 10:57:06.000000000 -0500
@@ -3301,6 +3301,7 @@ static ide_driver_t ide_cdrom_driver = {
 	.supports_dsc_overlap	= 1,
 	.cleanup		= ide_cdrom_cleanup,
 	.do_request		= ide_do_rw_cdrom,
+	.end_request		= cdrom_end_request,
 	.capacity		= ide_cdrom_capacity,
 	.attach			= ide_cdrom_attach,
 	.drives			=
LIST_HEAD_INIT(ide_cdrom_driver.drives),



> I should have explained this better.  DRIVER(drive)->end_request() is 
> actually calling default_end_request(), which calls ide_end_reuqest(),

> which ends up calling __ide_end_request().  And that will only end the

> request if end_that_request_first() (in ll_rw_blk.c) returns 0.
> However, end_that_request_first() returns 1 because rq->bio is not
NULL 
> (in the case of GPCMD_GET_CONFIGURATION, anyway).
> 
> So, the request isn't ended, but neither is rq->errors cleared.

RE: ide-cd question

[Stuart_Hayes]

> 
> I'd recommend applying this patch if nobody sees any problems with it.
> 
> 
> --- ide-cd.c.orig	2005-02-07 10:56:42.000000000 -0500
> +++ ide-cd.c	2005-02-07 10:57:06.000000000 -0500
> @@ -3301,6 +3301,7 @@ static ide_driver_t ide_cdrom_driver = {
>  	.supports_dsc_overlap	= 1,
>  	.cleanup		= ide_cdrom_cleanup,
>  	.do_request		= ide_do_rw_cdrom,
> +	.end_request		= cdrom_end_request,
>  	.capacity		= ide_cdrom_capacity,
>  	.attach			= ide_cdrom_attach,
>  	.drives			=
> LIST_HEAD_INIT(ide_cdrom_driver.drives),
> 
> 

I took the lack of responses to mean that either nobody cared, or the 
patch was so bad that it wasn't worthy of comment, so I took a closer 
look at it.  After looking at it more carefully, I suspect that the 
function cdrom_end_request() was meant to be an internal function, and 
not the end_request function for the driver (in part because the 
number of parameters for cdrom_end_request() isn't the same as 
ide_end_request()!).

Here's another patch that fixes the issue I'm seeing, and is less likely

to cause any other problems.  This will make sure that requests that are

ended in ide_atapi_error() because they failed too many times (even
after
a couple reset attempts) are completely ended, even if they didn't
finish 
transferring all the data that was expected.

Please apply if you don't see any problems with it.

Thanks!
Stuart



--- ide-io.c.orig	2005-02-10 15:23:52.000000000 -0500
+++ ide-io.c.new	2005-02-10 15:23:31.000000000 -0500
@@ -515,7 +515,18 @@ static ide_startstop_t ide_atapi_error(i
 		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
 
 	if (rq->errors >= ERROR_MAX) {
-		drive->driver->end_request(drive, 0, 0);
+		/* 
+		 * make sure request is fully ended--otherwise bio might
get updated and the
+		 * command will be retried without rq->errors getting
reset to zero, which
+		 * could cause us to get stuck in a loop with infinite
retries without any 
+		 * more reset attempts (borrowed from cdrom_end_request)
+		 */
+		int nsectors;
+		if (blk_pc_request(rq))
+			nsectors = (rq->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;
+		drive->driver->end_request(drive, 0, nsectors);
 	} else {
 		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
 			++rq->errors;

RE: ide-cd question

[Stuart_Hayes]

Hayes, Stuart wrote:
>> I'd recommend applying this patch if nobody sees any problems with
>> it. 
>> 
>> 
>> --- ide-cd.c.orig	2005-02-07 10:56:42.000000000 -0500
>> +++ ide-cd.c	2005-02-07 10:57:06.000000000 -0500
>> @@ -3301,6 +3301,7 @@ static ide_driver_t ide_cdrom_driver = { 
>>  	.supports_dsc_overlap	= 1, .cleanup		=
ide_cdrom_cleanup,
>>  	.do_request		= ide_do_rw_cdrom,
>> +	.end_request		= cdrom_end_request,
>>  	.capacity		= ide_cdrom_capacity,
>>  	.attach			= ide_cdrom_attach,
>>  	.drives			=
>> LIST_HEAD_INIT(ide_cdrom_driver.drives),
>> 
>> 
> 
> I took the lack of responses to mean that either nobody cared, or the
> patch was so bad that it wasn't worthy of comment, so I took a closer
> look at it.  After looking at it more carefully, I suspect that the
> function cdrom_end_request() was meant to be an internal function,
> and not the end_request function for the driver (in part because the
> number of parameters for cdrom_end_request() isn't the same as
> ide_end_request()!).      
> 
> Here's another patch that fixes the issue I'm seeing, and is less
> likely to cause any other problems.  This will make sure that
> requests that are ended in ide_atapi_error() because they failed too
> many times (even after a couple reset attempts) are completely ended,
> even if they didn't finish transferring all the data that was
> expected.     
> 
> Please apply if you don't see any problems with it.
> 
> Thanks!
> Stuart
> 
> 
> 
> --- ide-io.c.orig	2005-02-10 15:23:52.000000000 -0500
> +++ ide-io.c.new	2005-02-10 15:23:31.000000000 -0500
> @@ -515,7 +515,18 @@ static ide_startstop_t ide_atapi_error(i
>  		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
> 
>  	if (rq->errors >= ERROR_MAX) {
> -		drive->driver->end_request(drive, 0, 0);
> +		/*
> +		 * make sure request is fully ended--otherwise bio might
get
> updated and the +		 * command will be retried without
rq->errors
> getting reset to zero, which +		 * could cause us to get
stuck in a
> loop with infinite retries without any +		 * more reset
attempts
> (borrowed from cdrom_end_request) +		 */
> +		int nsectors;
> +		if (blk_pc_request(rq))
> +			nsectors = (rq->data_len + 511) >> 9;
> +		if (!nsectors)
> +			nsectors = 1;
> +		drive->driver->end_request(drive, 0, nsectors);
>  	} else {
>  		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
>  			++rq->errors;


Sorry--this patch is against 2.6.11-rc3.

Also sorry for the mangled patch--I'll resend it from another email
address
shortly.

Thanks
Stuart

Re: ide-cd question

[Stuart Hayes]

> Sorry--this patch is against 2.6.11-rc3.
>
> Also sorry for the mangled patch--I'll resend it from another email
> address shortly.
>
> Thanks
> Stuart

OK, here's that patch again--hopefully not mangled this time.
Thanks,
Stuart


--- ide-io.c.orig 2005-02-10 15:23:52.000000000 -0500
+++ ide-io.c.new 2005-02-10 15:23:31.000000000 -0500
@@ -515,7 +515,18 @@ static ide_startstop_t ide_atapi_error(i
   hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);

  if (rq->errors >= ERROR_MAX) {
-  drive->driver->end_request(drive, 0, 0);
+  /*
+   * make sure request is fully ended--otherwise bio might get updated and 
the
+   * command will be retried without rq->errors getting reset to zero, 
which
+   * could cause us to get stuck in a loop with infinite retries without 
any
+   * more reset attempts (borrowed from cdrom_end_request)
+   */
+  int nsectors;
+  if (blk_pc_request(rq))
+   nsectors = (rq->data_len + 511) >> 9;
+  if (!nsectors)
+   nsectors = 1;
+  drive->driver->end_request(drive, 0, nsectors);
  } else {
   if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
    ++rq->errors;

Re: ide-cd question

[Stuart Hayes]

>> Sorry--this patch is against 2.6.11-rc3.
>>
>> Also sorry for the mangled patch--I'll resend it from another email
>> address shortly.
>>
>> Thanks
>> Stuart
> 
> OK, here's that patch again--hopefully not mangled this time.
> Thanks,
> Stuart
> 
> 

This is frustrating.  Here it is again:


--- ide-io.c.orig 2005-02-10 15:23:52.000000000 -0500
+++ ide-io.c.new 2005-02-10 15:23:31.000000000 -0500
@@ -515,7 +515,18 @@ static ide_startstop_t ide_atapi_error(i
   hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
 
  if (rq->errors >= ERROR_MAX) {
-  drive->driver->end_request(drive, 0, 0);
+  /* 
+   * make sure request is fully ended--otherwise bio might get updated and the
+   * command will be retried without rq->errors getting reset to zero, which
+   * could cause us to get stuck in a loop with infinite retries without any 
+   * more reset attempts (borrowed from cdrom_end_request)
+   */
+  int nsectors;
+  if (blk_pc_request(rq))
+   nsectors = (rq->data_len + 511) >> 9;
+  if (!nsectors)
+   nsectors = 1;
+  drive->driver->end_request(drive, 0, nsectors);
  } else {
   if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
    ++rq->errors;

Re: ide-cd question

[Stuart Hayes]

I sent in this patch a few weeks ago, and never saw a response... I was 
wondering if there was a problem with it, or if I need to supply more 
info...?  This patch is against 2.6.11-rc3.  It makes sure that when 
ide_atapi_error() tries to end a failing ATAPI request after 2 reset 
attempts by calling drive->driver->end_request(), it will really be ended.  
Right now, a request that has a non-null rq->bio will not get ended, nor 
will rq->errors get cleared, so it will get retried forever with no more 
reset attempts.

My fingers are crossed that this patch won't be mangled... I seem to have 
a problem with that...

Thanks,
Stuart


--- ide-io.c.orig 2005-02-10 15:23:52.000000000 -0500
+++ ide-io.c.new 2005-02-10 15:23:31.000000000 -0500
@@ -515,7 +515,18 @@ static ide_startstop_t ide_atapi_error(i
   hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
 
  if (rq->errors >= ERROR_MAX) {
-  drive->driver->end_request(drive, 0, 0);
+  /* 
+   * make sure request is fully ended--otherwise bio might get updated and the
+   * command will be retried without rq->errors getting reset to zero, which
+   * could cause us to get stuck in a loop with infinite retries without any 
+   * more reset attempts (borrowed from cdrom_end_request)
+   */
+  int nsectors;
+  if (blk_pc_request(rq))
+   nsectors = (rq->data_len + 511) >> 9;
+  if (!nsectors)
+   nsectors = 1;
+  drive->driver->end_request(drive, 0, nsectors);
  } else {
   if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
    ++rq->errors;

Re: ide-cd question

[Jens Axboe]

On Thu, Mar 03 2005, Stuart Hayes wrote:
> I sent in this patch a few weeks ago, and never saw a response... I was 
> wondering if there was a problem with it, or if I need to supply more 
> info...?  This patch is against 2.6.11-rc3.  It makes sure that when 
> ide_atapi_error() tries to end a failing ATAPI request after 2 reset 
> attempts by calling drive->driver->end_request(), it will really be ended.  
> Right now, a request that has a non-null rq->bio will not get ended, nor 
> will rq->errors get cleared, so it will get retried forever with no more 
> reset attempts.

Indeed a problem. Would be nice to switch the ->end_request() to be byte
based like SCSI, would make it cleaner.

But do you really need the !nsectors check? If ->data_len is 0, there
should not be a need to pass a non-zero sector count.

> My fingers are crossed that this patch won't be mangled... I seem to have 
> a problem with that...

No such luck, it's mangled :). Looks like tabs turned to a single space.
Maybe you have better luck with attaching the file.

-- 
Jens Axboe

RE: ide-cd question

[Stuart_Hayes]

Jens Axboe wrote:
> On Thu, Mar 03 2005, Stuart Hayes wrote:
>> I sent in this patch a few weeks ago, and never saw a response... I
>> was wondering if there was a problem with it, or if I need to supply
>> more info...?  This patch is against 2.6.11-rc3.  It makes sure that
>> when
>> ide_atapi_error() tries to end a failing ATAPI request after 2 reset
>> attempts by calling drive->driver->end_request(), it will really be
>> ended. Right now, a request that has a non-null rq->bio will not get
>> ended, 
>> nor will rq->errors get cleared, so it will get retried forever with
>> no more reset attempts.
> 
> Indeed a problem. Would be nice to switch the ->end_request() to be
> byte based like SCSI, would make it cleaner. 
> 
> But do you really need the !nsectors check? If ->data_len is 0, there
> should not be a need to pass a non-zero sector count. 
> 

I wondered about the !nsectors check myself.  I copied that code from
the cdrom_end_request() function, and I assumed that it must have been
there for some reason that wasn't immediately clear to me, so I didn't
take it out.  I figured performance in that code path wasn't critical.

I can take it out if that's the only problem!

>> My fingers are crossed that this patch won't be mangled... I seem to
>> have a problem with that...
> 
> No such luck, it's mangled :). Looks like tabs turned to a single
> space. 
> Maybe you have better luck with attaching the file.

Here's another try at the patch.  This time it is against 2.6.11, and I
shortened the comments so I could send it from this email address 
without it getting mangled...



--- ide-io.c.orig	2005-03-03 10:57:05.468669136 -0500
+++ ide-io.c	2005-03-03 11:01:33.982848776 -0500
@@ -486,9 +486,21 @@ static ide_startstop_t ide_ata_error(ide
 		/* force an abort */
 		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
 
-	if (rq->errors >= ERROR_MAX || blk_noretry_request(rq))
-		drive->driver->end_request(drive, 0, 0);
-	else {
+	if (rq->errors >= ERROR_MAX || blk_noretry_request(rq)) {
+		/* 
+		 * make sure request is fully ended--otherwise bio might
get
+		 * updated and the command will be retried without
+		 * rq->errors getting reset to zero, which could cause
us to
+		 * get stuckin a loop with infinite retries without any
more
+		 * reset attempts (borrowed from cdrom_end_request)
+		 */
+		int nsectors;
+		if (blk_pc_request(rq))
+			nsectors = (rq->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;
+		drive->driver->end_request(drive, 0, nsectors);
+	} else {
 		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
 			++rq->errors;
 			return ide_do_reset(drive);

Re: ide-cd question

[Jens Axboe]

On Thu, Mar 03 2005, Stuart_Hayes@Dell.com wrote:
> Jens Axboe wrote:
> > On Thu, Mar 03 2005, Stuart Hayes wrote:
> >> I sent in this patch a few weeks ago, and never saw a response... I
> >> was wondering if there was a problem with it, or if I need to supply
> >> more info...?  This patch is against 2.6.11-rc3.  It makes sure that
> >> when
> >> ide_atapi_error() tries to end a failing ATAPI request after 2 reset
> >> attempts by calling drive->driver->end_request(), it will really be
> >> ended. Right now, a request that has a non-null rq->bio will not get
> >> ended, 
> >> nor will rq->errors get cleared, so it will get retried forever with
> >> no more reset attempts.
> > 
> > Indeed a problem. Would be nice to switch the ->end_request() to be
> > byte based like SCSI, would make it cleaner. 
> > 
> > But do you really need the !nsectors check? If ->data_len is 0, there
> > should not be a need to pass a non-zero sector count. 
> > 
> 
> I wondered about the !nsectors check myself.  I copied that code from
> the cdrom_end_request() function, and I assumed that it must have been
> there for some reason that wasn't immediately clear to me, so I didn't
> take it out.  I figured performance in that code path wasn't critical.
> 
> I can take it out if that's the only problem!

Lets just keep it, it sure doesn't hurt :)

> >> My fingers are crossed that this patch won't be mangled... I seem to
> >> have a problem with that...
> > 
> > No such luck, it's mangled :). Looks like tabs turned to a single
> > space. 
> > Maybe you have better luck with attaching the file.
> 
> Here's another try at the patch.  This time it is against 2.6.11, and I
> shortened the comments so I could send it from this email address 
> without it getting mangled...
> 
> 
> 
> --- ide-io.c.orig	2005-03-03 10:57:05.468669136 -0500
> +++ ide-io.c	2005-03-03 11:01:33.982848776 -0500
> @@ -486,9 +486,21 @@ static ide_startstop_t ide_ata_error(ide
>  		/* force an abort */
>  		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
>  
> -	if (rq->errors >= ERROR_MAX || blk_noretry_request(rq))
> -		drive->driver->end_request(drive, 0, 0);
> -	else {
> +	if (rq->errors >= ERROR_MAX || blk_noretry_request(rq)) {
> +		/* 
> +		 * make sure request is fully ended--otherwise bio might
> get
> +		 * updated and the command will be retried without
> +		 * rq->errors getting reset to zero, which could cause
> us to
> +		 * get stuckin a loop with infinite retries without any
> more
> +		 * reset attempts (borrowed from cdrom_end_request)
> +		 */
> +		int nsectors;
> +		if (blk_pc_request(rq))
> +			nsectors = (rq->data_len + 511) >> 9;
> +		if (!nsectors)
> +			nsectors = 1;
> +		drive->driver->end_request(drive, 0, nsectors);

There's still a problem here, you are not initializing nsectors for
non-pc requests. And your comments wrap :)

        int nsectors = rq->hard_nr_sectors;

        if (blk_pc_request(rq))
                nsectors = (rq->data_len + 511) >> 9;
        if (!nsectors)
                nsectors = 1;

        ...

Can you resend with that fixed up and with a Signed-off-by header?

-- 
Jens Axboe

RE: ide-cd question

[Stuart_Hayes]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]


> 
> There's still a problem here, you are not initializing nsectors for
> non-pc requests. And your comments wrap :) 
> 
>         int nsectors = rq->hard_nr_sectors;
> 
>         if (blk_pc_request(rq))
>                 nsectors = (rq->data_len + 511) >> 9;
>         if (!nsectors)
>                 nsectors = 1;
> 
>         ...
> 
> Can you resend with that fixed up and with a Signed-off-by header?

Here it is.  Thanks for all the help.  I'm attaching as a file, too,
just in case it gets garbled again.
Stuart


Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>

--- linux-2.6.11/drivers/ide/ide-io.c.orig	2005-03-04
16:11:14.000000000 -0500
+++ linux-2.6.11/drivers/ide/ide-io.c	2005-03-04 16:19:19.000000000
-0500
@@ -516,7 +516,19 @@ static ide_startstop_t ide_atapi_error(i
 		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
 
 	if (rq->errors >= ERROR_MAX) {
-		drive->driver->end_request(drive, 0, 0);
+		/* 
+		 * make sure request is fully ended--otherwise the 
+		 * command will be retried without rq->errors getting
+		 * reset to zero, which could cause us to get stuck
+		 * in a loop with infinite retries without any more
+		 * reset attempts
+		 */
+		int nsectors = rq->hard_nr_sectors;
+		if (blk_pc_request(rq))
+			nsectors = (rq->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;
+		drive->driver->end_request(drive, 0, nsectors);
 	} else {
 		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
 			++rq->errors;

[-- Attachment #2: atapi_max_errors_d_2.6.11.patch --]
[-- Type: application/octet-stream, Size: 878 bytes --]

--- linux-2.6.11/drivers/ide/ide-io.c.orig	2005-03-04 16:11:14.000000000 -0500
+++ linux-2.6.11/drivers/ide/ide-io.c	2005-03-04 16:19:19.000000000 -0500
@@ -516,7 +516,19 @@ static ide_startstop_t ide_atapi_error(i
 		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
 
 	if (rq->errors >= ERROR_MAX) {
-		drive->driver->end_request(drive, 0, 0);
+		/* 
+		 * make sure request is fully ended--otherwise the 
+		 * command will be retried without rq->errors getting
+		 * reset to zero, which could cause us to get stuck
+		 * in a loop with infinite retries without any more
+		 * reset attempts
+		 */
+		int nsectors = rq->hard_nr_sectors;
+		if (blk_pc_request(rq))
+			nsectors = (rq->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;
+		drive->driver->end_request(drive, 0, nsectors);
 	} else {
 		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
 			++rq->errors;

Re: ide-cd question

[Jens Axboe]

On Fri, Mar 04 2005, Stuart_Hayes@Dell.com wrote:
> 
> > 
> > There's still a problem here, you are not initializing nsectors for
> > non-pc requests. And your comments wrap :) 
> > 
> >         int nsectors = rq->hard_nr_sectors;
> > 
> >         if (blk_pc_request(rq))
> >                 nsectors = (rq->data_len + 511) >> 9;
> >         if (!nsectors)
> >                 nsectors = 1;
> > 
> >         ...
> > 
> > Can you resend with that fixed up and with a Signed-off-by header?
> 
> Here it is.  Thanks for all the help.  I'm attaching as a file, too,
> just in case it gets garbled again.
> Stuart
> 
> 
> Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>
> 
> --- linux-2.6.11/drivers/ide/ide-io.c.orig	2005-03-04
> 16:11:14.000000000 -0500
> +++ linux-2.6.11/drivers/ide/ide-io.c	2005-03-04 16:19:19.000000000
> -0500
> @@ -516,7 +516,19 @@ static ide_startstop_t ide_atapi_error(i
>  		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
>  
>  	if (rq->errors >= ERROR_MAX) {
> -		drive->driver->end_request(drive, 0, 0);
> +		/* 
> +		 * make sure request is fully ended--otherwise the 
> +		 * command will be retried without rq->errors getting
> +		 * reset to zero, which could cause us to get stuck
> +		 * in a loop with infinite retries without any more
> +		 * reset attempts
> +		 */
> +		int nsectors = rq->hard_nr_sectors;
> +		if (blk_pc_request(rq))
> +			nsectors = (rq->data_len + 511) >> 9;
> +		if (!nsectors)
> +			nsectors = 1;
> +		drive->driver->end_request(drive, 0, nsectors);
>  	} else {
>  		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
>  			++rq->errors;

Looks good, Bart care to pick this up?

Signed-off-by: Jens Axboe <axboe@suse.de>

-- 
Jens Axboe

RE: ide-cd question

[Stuart_Hayes]

Jens Axboe wrote:
> On Fri, Mar 04 2005, Stuart_Hayes@Dell.com wrote:
>> 
>>> 
>>> There's still a problem here, you are not initializing nsectors for
>>> non-pc requests. And your comments wrap :)
>>> 
>>>         int nsectors = rq->hard_nr_sectors;
>>> 
>>>         if (blk_pc_request(rq))
>>>                 nsectors = (rq->data_len + 511) >> 9;         if
>>>                 (!nsectors) nsectors = 1;
>>> 
>>>         ...
>>> 
>>> Can you resend with that fixed up and with a Signed-off-by header?
>> 
>> Here it is.  Thanks for all the help.  I'm attaching as a file, too,
>> just in case it gets garbled again.
>> Stuart
>> 
>> 
>> Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>
>> 
>> --- linux-2.6.11/drivers/ide/ide-io.c.orig	2005-03-04
>> 16:11:14.000000000 -0500 +++
>> linux-2.6.11/drivers/ide/ide-io.c	2005-03-04 16:19:19.000000000
>>  		-0500 @@ -516,7 +516,19 @@ static ide_startstop_t
>> ide_atapi_error(i hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG); 
>> 
>>  	if (rq->errors >= ERROR_MAX) {
>> -		drive->driver->end_request(drive, 0, 0);
>> +		/*
>> +		 * make sure request is fully ended--otherwise the
>> +		 * command will be retried without rq->errors getting
>> +		 * reset to zero, which could cause us to get stuck
>> +		 * in a loop with infinite retries without any more +
* reset
>> attempts +		 */
>> +		int nsectors = rq->hard_nr_sectors;
>> +		if (blk_pc_request(rq))
>> +			nsectors = (rq->data_len + 511) >> 9;
>> +		if (!nsectors)
>> +			nsectors = 1;
>> +		drive->driver->end_request(drive, 0, nsectors);
>>  	} else {
>>  		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
>>  			++rq->errors;
> 
> Looks good, Bart care to pick this up?
> 
> Signed-off-by: Jens Axboe <axboe@suse.de>

Bart--

Is there a chance you might pick this up, or is there a problem with it?

Thanks
Stuart

Re: ide-cd question

[Bartlomiej Zolnierkiewicz]

On Thu, 17 Mar 2005 10:17:58 -0600, Stuart_Hayes@dell.com
<Stuart_Hayes@dell.com> wrote:
> Jens Axboe wrote:
> > On Fri, Mar 04 2005, Stuart_Hayes@Dell.com wrote:
> >>
> >>>
> >>> There's still a problem here, you are not initializing nsectors for
> >>> non-pc requests. And your comments wrap :)
> >>>
> >>>         int nsectors = rq->hard_nr_sectors;
> >>>
> >>>         if (blk_pc_request(rq))
> >>>                 nsectors = (rq->data_len + 511) >> 9;         if
> >>>                 (!nsectors) nsectors = 1;
> >>>
> >>>         ...
> >>>
> >>> Can you resend with that fixed up and with a Signed-off-by header?
> >>
> >> Here it is.  Thanks for all the help.  I'm attaching as a file, too,
> >> just in case it gets garbled again.
> >> Stuart
> >>
> >>
> >> Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>
> >>
> >> --- linux-2.6.11/drivers/ide/ide-io.c.orig   2005-03-04
> >> 16:11:14.000000000 -0500 +++
> >> linux-2.6.11/drivers/ide/ide-io.c    2005-03-04 16:19:19.000000000
> >>              -0500 @@ -516,7 +516,19 @@ static ide_startstop_t
> >> ide_atapi_error(i hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
> >>
> >>      if (rq->errors >= ERROR_MAX) {
> >> -            drive->driver->end_request(drive, 0, 0);
> >> +            /*
> >> +             * make sure request is fully ended--otherwise the
> >> +             * command will be retried without rq->errors getting
> >> +             * reset to zero, which could cause us to get stuck
> >> +             * in a loop with infinite retries without any more +
> * reset
> >> attempts +            */
> >> +            int nsectors = rq->hard_nr_sectors;
> >> +            if (blk_pc_request(rq))
> >> +                    nsectors = (rq->data_len + 511) >> 9;
> >> +            if (!nsectors)
> >> +                    nsectors = 1;

ide_end_request() handles this case differently
and this code path is used not only by ide-cd.

You've changed the current behavior here.
The question is - what are the consequences of that?

> >> +            drive->driver->end_request(drive, 0, nsectors);
> >>      } else {
> >>              if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
> >>                      ++rq->errors;
> >
> > Looks good, Bart care to pick this up?

This is not so obvious for me...

> > Signed-off-by: Jens Axboe <axboe@suse.de>
> 
> Bart--
> 
> Is there a chance you might pick this up, or is there a problem with it?
> 
> Thanks
> Stuart
>

RE: ide-cd question

[Stuart_Hayes]

Bartlomiej Zolnierkiewicz wrote:
> On Thu, 17 Mar 2005 10:17:58 -0600, Stuart_Hayes@dell.com
> <Stuart_Hayes@dell.com> wrote: 
>> Jens Axboe wrote:
>>> On Fri, Mar 04 2005, Stuart_Hayes@Dell.com wrote:
>>>> 
>>>>> 
>>>>> There's still a problem here, you are not initializing nsectors
>>>>> for non-pc requests. And your comments wrap :)
>>>>> 
>>>>>         int nsectors = rq->hard_nr_sectors;
>>>>> 
>>>>>         if (blk_pc_request(rq))
>>>>>                 nsectors = (rq->data_len + 511) >> 9;         if
>>>>>                 (!nsectors) nsectors = 1;
>>>>> 
>>>>>         ...
>>>>> 
>>>>> Can you resend with that fixed up and with a Signed-off-by header?
>>>> 
>>>> Here it is.  Thanks for all the help.  I'm attaching as a file,
>>>> too, just in case it gets garbled again.
>>>> Stuart
>>>> 
>>>> 
>>>> Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>
>>>> 
>>>> --- linux-2.6.11/drivers/ide/ide-io.c.orig   2005-03-04
>>>> 16:11:14.000000000 -0500 +++ linux-2.6.11/drivers/ide/ide-io.c   
>>>>              2005-03-04 16:19:19.000000000 -0500 @@ -516,7 +516,19
>>>> @@ static ide_startstop_t ide_atapi_error(i
>>>> hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG); 
>>>> 
>>>>      if (rq->errors >= ERROR_MAX) {
>>>> -            drive->driver->end_request(drive, 0, 0); +           
>>>> /* +             * make sure request is fully ended--otherwise the
>>>> +             * command will be retried without rq->errors getting
>>>> +             * reset to zero, which could cause us to get stuck
>>>> +             * in a loop with infinite retries without any more +
>>>> * reset attempts +            */ +            int nsectors =
>>>> rq->hard_nr_sectors; +            if (blk_pc_request(rq))
>>>> +                    nsectors = (rq->data_len + 511) >> 9; +      
>>>> if (!nsectors) +                    nsectors = 1;
> 
> ide_end_request() handles this case differently and this code path is
> used not only by ide-cd. 
> 
> You've changed the current behavior here.

The idea is to make sure that ide_end_request() actually does end the
request.

I don't think I ever said, but the problem I'm seeing occurs when an
IDE CD stops responding at all--it is as if it was unplugged from 
the system.  In this case, the busy bit in the status register reads
as "1", so the command never gets past the ide_wait_stat() that's
done in start_request before do_request() is called.

ide_wait_stat() then calls ide_atapi_error(), which does its thing, 
and the command goes back to start_request... after a couple loops of 
this, ide_atapi_error() decides to give up and end the request... but
ide_end_request() *isn't* ending the request, and it is stuck in the
queue getting retried forever, which doesn't seem desirable.

The patch will make ide_end_request actually end the request, though
I admit it isn't a particularly elegant way of doing so.

Do you agree that this is worth fixing?  If so, do you have any
suggestion as to how else it might be fixed?  It seems to me that
this would be a good thing to do for any device, not just CD-ROMs.


> The question is - what are the consequences of that?
> 
>>>> +            drive->driver->end_request(drive, 0, nsectors);     
>>>>              } else { if ((rq->errors & ERROR_RESET) ==
>>>>                      ERROR_RESET) { ++rq->errors;
>>> 
>>> Looks good, Bart care to pick this up?
> 
> This is not so obvious for me...
> 
>>> Signed-off-by: Jens Axboe <axboe@suse.de>
>> 
>> Bart--
>> 
>> Is there a chance you might pick this up, or is there a problem with
>> it? 
>> 
>> Thanks
>> Stuart