[PATCH] Improve code for detecting errors near the end of a CD

[PATCH] Improve code for detecting errors near the end of a CD

[Alan Stern]

James:

This revised patch (as572b) improves the code in sr.c that detects where
read/write errors occurred and moves it to a separate (but inline) routine
for greater clarity.

The original code contained a glaring mistake: testing
(SCpnt->sense_buffer[0] & 0x90) to see if the Information field in the
sense data is valid.  The Valid bit is 0x80; the test against 0x90 can
never fail, thanks to an earlier test: (SCpnt->sense_buffer[0] & 0x7f) ==
0x70).

The patch is careful to check that the error sector is within the range of
blocks that were supposed to be transferred, and it rounds the reported 
number of bytes transferred down to a multiple of the block layer's block 
size.

The major enhancement is that the patch uses the Residue to calculate the 
error sector, if the sense data doesn't already provide it.  This helps 
with a drive I use.  Since the Residue isn't always reported correctly, 
the patch is careful to check that it has a reasonable value: somewhere 
strictly between 0 and the total transfer length.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/scsi/sr.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sr.c
+++ usb-2.6/drivers/scsi/sr.c
@@ -203,7 +203,44 @@ int sr_media_change(struct cdrom_device_
 	}
 	return retval;
 }
- 
+
+/* 
+ * Use the information from a failed command to compute how much data
+ * was transferred successfully, and find the sector that caused the
+ * error.  Be careful to avoid errors from numerical overflow.
+ */
+static inline int calc_good_bytes(struct scsi_cmnd *SCpnt,
+		long *error_sector, int hw_blocksize,
+		int sectors_per_bio_block)
+{
+	int req_bytes = SCpnt->bufflen;
+	unsigned int sectors = 0;
+
+	if (SCpnt->sense_buffer[0] & 0x80) {
+		/* The Information field is valid; use it */
+		unsigned int start_block, error_block, blocks;
+
+		start_block = (unsigned int) SCpnt->request->sector /
+				(hw_blocksize >> 9);
+		error_block = (SCpnt->sense_buffer[3] << 24) |
+				(SCpnt->sense_buffer[4] << 16) |
+				(SCpnt->sense_buffer[5] << 8) |
+				SCpnt->sense_buffer[6];
+		blocks = error_block - start_block;
+		if (blocks < req_bytes / hw_blocksize)
+			sectors = blocks * (hw_blocksize >> 9);
+	} else {
+		/* Fall back on the Residue */
+		if (SCpnt->resid > 0 && SCpnt->resid < req_bytes)
+			sectors = (req_bytes - SCpnt->resid) >> 9;
+	}
+	*error_sector = SCpnt->request->sector + sectors;
+
+	/* Round the amount to a multiple of the block layer's block size */
+	sectors &= ~(sectors_per_bio_block - 1);
+	return sectors << 9;
+}
+
 /*
  * rw_intr is the interrupt routine for the device driver.
  *
@@ -235,25 +272,17 @@ static void rw_intr(struct scsi_cmnd * S
 		case MEDIUM_ERROR:
 		case VOLUME_OVERFLOW:
 		case ILLEGAL_REQUEST:
-			if (!(SCpnt->sense_buffer[0] & 0x90))
-				break;
 			if (!blk_fs_request(SCpnt->request))
 				break;
-			error_sector = (SCpnt->sense_buffer[3] << 24) |
-				(SCpnt->sense_buffer[4] << 16) |
-				(SCpnt->sense_buffer[5] << 8) |
-				SCpnt->sense_buffer[6];
 			if (SCpnt->request->bio != NULL)
 				block_sectors =
 					bio_sectors(SCpnt->request->bio);
 			if (block_sectors < 4)
 				block_sectors = 4;
-			if (cd->device->sector_size == 2048)
-				error_sector <<= 2;
-			error_sector &= ~(block_sectors - 1);
-			good_bytes = (error_sector - SCpnt->request->sector) << 9;
-			if (good_bytes < 0 || good_bytes >= this_count)
-				good_bytes = 0;
+
+			good_bytes = calc_good_bytes(SCpnt, &error_sector,
+					cd->device->sector_size,
+					block_sectors);
 			/*
 			 * The SCSI specification allows for the value
 			 * returned by READ CAPACITY to be up to 75 2K

Re: [PATCH] Improve code for detecting errors near the end of a CD

[Douglas Gilbert]

Alan Stern wrote:
> James:
> 
> This revised patch (as572b) improves the code in sr.c that detects where
> read/write errors occurred and moves it to a separate (but inline) routine
> for greater clarity.
> 
> The original code contained a glaring mistake: testing
> (SCpnt->sense_buffer[0] & 0x90) to see if the Information field in the
> sense data is valid.  The Valid bit is 0x80; the test against 0x90 can
> never fail, thanks to an earlier test: (SCpnt->sense_buffer[0] & 0x7f) ==
> 0x70).
> 
> The patch is careful to check that the error sector is within the range of
> blocks that were supposed to be transferred, and it rounds the reported 
> number of bytes transferred down to a multiple of the block layer's block 
> size.
> 
> The major enhancement is that the patch uses the Residue to calculate the 
> error sector, if the sense data doesn't already provide it.  This helps 
> with a drive I use.  Since the Residue isn't always reported correctly, 
> the patch is careful to check that it has a reasonable value: somewhere 
> strictly between 0 and the total transfer length.

Alan,
In include/scsi/scsi_eh.h there are several helper functions
to aid processing SCSI errors. This includes SCSI sense data
descriptor format (which won't be needed for DVD/HD/BD for
some time with a (2**32 * 2048) byte maximum using existing
fixed sense data format). However there is
scsi_get_sense_info_fld() to fetch the info field.

sd, st and sg have been converted to use these helpers,
where appropriate.

MMC-4 does not mention that the valid bit needs to
be set on a MEDIUM/HARDWARE error and I have seen
real life examples of this. [So it's poorly defined
if one gets a medium error on lba 0.] You may also like to
consider deferred errors which can occur according to
MMC-4.

Doug Gilbert

> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Index: usb-2.6/drivers/scsi/sr.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/sr.c
> +++ usb-2.6/drivers/scsi/sr.c
> @@ -203,7 +203,44 @@ int sr_media_change(struct cdrom_device_
>  	}
>  	return retval;
>  }
> - 
> +
> +/* 
> + * Use the information from a failed command to compute how much data
> + * was transferred successfully, and find the sector that caused the
> + * error.  Be careful to avoid errors from numerical overflow.
> + */
> +static inline int calc_good_bytes(struct scsi_cmnd *SCpnt,
> +		long *error_sector, int hw_blocksize,
> +		int sectors_per_bio_block)
> +{
> +	int req_bytes = SCpnt->bufflen;
> +	unsigned int sectors = 0;
> +
> +	if (SCpnt->sense_buffer[0] & 0x80) {
> +		/* The Information field is valid; use it */
> +		unsigned int start_block, error_block, blocks;
> +
> +		start_block = (unsigned int) SCpnt->request->sector /
> +				(hw_blocksize >> 9);
> +		error_block = (SCpnt->sense_buffer[3] << 24) |
> +				(SCpnt->sense_buffer[4] << 16) |
> +				(SCpnt->sense_buffer[5] << 8) |
> +				SCpnt->sense_buffer[6];
> +		blocks = error_block - start_block;
> +		if (blocks < req_bytes / hw_blocksize)
> +			sectors = blocks * (hw_blocksize >> 9);
> +	} else {
> +		/* Fall back on the Residue */
> +		if (SCpnt->resid > 0 && SCpnt->resid < req_bytes)
> +			sectors = (req_bytes - SCpnt->resid) >> 9;
> +	}
> +	*error_sector = SCpnt->request->sector + sectors;
> +
> +	/* Round the amount to a multiple of the block layer's block size */
> +	sectors &= ~(sectors_per_bio_block - 1);
> +	return sectors << 9;
> +}
> +
>  /*
>   * rw_intr is the interrupt routine for the device driver.
>   *
> @@ -235,25 +272,17 @@ static void rw_intr(struct scsi_cmnd * S
>  		case MEDIUM_ERROR:
>  		case VOLUME_OVERFLOW:
>  		case ILLEGAL_REQUEST:
> -			if (!(SCpnt->sense_buffer[0] & 0x90))
> -				break;
>  			if (!blk_fs_request(SCpnt->request))
>  				break;
> -			error_sector = (SCpnt->sense_buffer[3] << 24) |
> -				(SCpnt->sense_buffer[4] << 16) |
> -				(SCpnt->sense_buffer[5] << 8) |
> -				SCpnt->sense_buffer[6];
>  			if (SCpnt->request->bio != NULL)
>  				block_sectors =
>  					bio_sectors(SCpnt->request->bio);
>  			if (block_sectors < 4)
>  				block_sectors = 4;
> -			if (cd->device->sector_size == 2048)
> -				error_sector <<= 2;
> -			error_sector &= ~(block_sectors - 1);
> -			good_bytes = (error_sector - SCpnt->request->sector) << 9;
> -			if (good_bytes < 0 || good_bytes >= this_count)
> -				good_bytes = 0;
> +
> +			good_bytes = calc_good_bytes(SCpnt, &error_sector,
> +					cd->device->sector_size,
> +					block_sectors);
>  			/*
>  			 * The SCSI specification allows for the value
>  			 * returned by READ CAPACITY to be up to 75 2K
> 
> -
> 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: [PATCH] Improve code for detecting errors near the end of a CD

[Alan Stern]

On Sat, 15 Oct 2005, Douglas Gilbert wrote:

> Alan,
> In include/scsi/scsi_eh.h there are several helper functions
> to aid processing SCSI errors. This includes SCSI sense data
> descriptor format (which won't be needed for DVD/HD/BD for
> some time with a (2**32 * 2048) byte maximum using existing
> fixed sense data format). However there is
> scsi_get_sense_info_fld() to fetch the info field.
> 
> sd, st and sg have been converted to use these helpers,
> where appropriate.
> 
> MMC-4 does not mention that the valid bit needs to
> be set on a MEDIUM/HARDWARE error and I have seen
> real life examples of this. [So it's poorly defined
> if one gets a medium error on lba 0.] You may also like to
> consider deferred errors which can occur according to
> MMC-4.

Thanks for the comments.  It will be easiest to start with this patch and 
then add others on top.  I wouldn't mind writing another update to use 
scsi_get_sense_info_fld and friends.

I'm not familiar with the MMC standards and it will take some time to get 
up to speed on MMC-4.  I just downloaded the draft standard, and it's not 
obvious where (if anywhere) it describes the sense data values.

As for deferred errors, I don't know the proper way of handling them.  It 
would be better for someone else to take care of it.

Alan Stern

Re: [PATCH] Improve code for detecting errors near the end of a CD

[Douglas Gilbert]

Alan Stern wrote:
> On Sat, 15 Oct 2005, Douglas Gilbert wrote:
> 
> 
>>Alan,
>>In include/scsi/scsi_eh.h there are several helper functions
>>to aid processing SCSI errors. This includes SCSI sense data
>>descriptor format (which won't be needed for DVD/HD/BD for
>>some time with a (2**32 * 2048) byte maximum using existing
>>fixed sense data format). However there is
>>scsi_get_sense_info_fld() to fetch the info field.
>>
>>sd, st and sg have been converted to use these helpers,
>>where appropriate.
>>
>>MMC-4 does not mention that the valid bit needs to
>>be set on a MEDIUM/HARDWARE error and I have seen
>>real life examples of this. [So it's poorly defined
>>if one gets a medium error on lba 0.] You may also like to
>>consider deferred errors which can occur according to
>>MMC-4.
> 
> 
> Thanks for the comments.  It will be easiest to start with this patch and 
> then add others on top.  I wouldn't mind writing another update to use 
> scsi_get_sense_info_fld and friends.
> 
> I'm not familiar with the MMC standards and it will take some time to get 
> up to speed on MMC-4.  I just downloaded the draft standard, and it's not 
> obvious where (if anywhere) it describes the sense data values.

Alan,
With mmc4r05a.pdf search for "medium error". It talks
about setting the info field but says nothing about
the valid bit. In similar situations sbc (disks) and
ssc (tapes) do mention the valid bit. They should
all conform to spc (primary commands) but at the
margin (especially mmc) they don't. I have brought
up the subject on the "t10@t10.org" reflector, the
response, if memory serves, was along the lines of
"yep, live with it".

> As for deferred errors, I don't know the proper way of handling them.  It 
> would be better for someone else to take care of it.

While searching in the pdf you could look for
"deferred" as well. Lots of hits. Hard to say what
exactly to do, but reporting something to the log
wouldn't be a bad start. This file:
include/scsi/scsi_dbg.h
also contains sense data helper declarations.


Aside ... not addressed to Alan
Why this function is in scsi_dbg.h and is declared
like this:
extern const char *scsi_extd_sense_format(
		unsigned char, unsigned char);
rather than something like this:
extern const char *scsi_asc_ascq_str(
		unsigned char asc, unsigned char ascq);
is beyond me. What has "_extd_" got to do with it?
Basically even when I know the function is there,
and publically available, it is still difficult
to find!

Doug Gilbert

Re: [PATCH] Improve code for detecting errors near the end of a CD

[Alan Stern]

On Sat, 15 Oct 2005, Douglas Gilbert wrote:

> Alan,
> In include/scsi/scsi_eh.h there are several helper functions
> to aid processing SCSI errors. This includes SCSI sense data
> descriptor format (which won't be needed for DVD/HD/BD for
> some time with a (2**32 * 2048) byte maximum using existing
> fixed sense data format). However there is
> scsi_get_sense_info_fld() to fetch the info field.
> 
> sd, st and sg have been converted to use these helpers,
> where appropriate.
> 
> MMC-4 does not mention that the valid bit needs to
> be set on a MEDIUM/HARDWARE error and I have seen
> real life examples of this. [So it's poorly defined
> if one gets a medium error on lba 0.] You may also like to
> consider deferred errors which can occur according to
> MMC-4.

I just read the scsi_get_sense_info_fld routine.  For fixed format data, 
it won't return anything if the Valid bit isn't set!

This makes it hard to adopt both your first two suggestions... :-)

Alan Stern

Re: [PATCH] Improve code for detecting errors near the end of a CD

[Douglas Gilbert]

Alan Stern wrote:
> On Sat, 15 Oct 2005, Douglas Gilbert wrote:
> 
> 
>>Alan,
>>In include/scsi/scsi_eh.h there are several helper functions
>>to aid processing SCSI errors. This includes SCSI sense data
>>descriptor format (which won't be needed for DVD/HD/BD for
>>some time with a (2**32 * 2048) byte maximum using existing
>>fixed sense data format). However there is
>>scsi_get_sense_info_fld() to fetch the info field.
>>
>>sd, st and sg have been converted to use these helpers,
>>where appropriate.
>>
>>MMC-4 does not mention that the valid bit needs to
>>be set on a MEDIUM/HARDWARE error and I have seen
>>real life examples of this. [So it's poorly defined
>>if one gets a medium error on lba 0.] You may also like to
>>consider deferred errors which can occur according to
>>MMC-4.
> 
> 
> I just read the scsi_get_sense_info_fld routine.  For fixed format data, 
> it won't return anything if the Valid bit isn't set!
> 
> This makes it hard to adopt both your first two suggestions... :-)

Alan,
Ouch.
Here is the linux kernel version (scsi_error.c):

    case 0x70:
    case 0x71:
        if (sense_buffer[0] & 0x80) {
            *info_out = (sense_buffer[3] << 24) +
                        (sense_buffer[4] << 16) +
                        (sense_buffer[5] << 8) +
                        sense_buffer[6];
            return 1;
        } else
            return 0;

and here is the sg3_utils version (sg_lib.c):

    case 0x70:
    case 0x71:
        if (info_outp)
            *info_outp = (sensep[3] << 24) + (sensep[4] << 16) +
                         (sensep[5] << 8) + sensep[6];
        return (sensep[0] & 0x80) ? 1 : 0;

The latter implementation takes account of MMC devices.
So I probably need to send a patch.

Doug Gilbert

Re: [PATCH] Improve code for detecting errors near the end of a CD

[Alan Stern]

On Thu, 20 Oct 2005, Douglas Gilbert wrote:

> Alan,
> Ouch.
> Here is the linux kernel version (scsi_error.c):
> 
>     case 0x70:
>     case 0x71:
>         if (sense_buffer[0] & 0x80) {
>             *info_out = (sense_buffer[3] << 24) +
>                         (sense_buffer[4] << 16) +
>                         (sense_buffer[5] << 8) +
>                         sense_buffer[6];
>             return 1;
>         } else
>             return 0;
> 
> and here is the sg3_utils version (sg_lib.c):
> 
>     case 0x70:
>     case 0x71:
>         if (info_outp)
>             *info_outp = (sensep[3] << 24) + (sensep[4] << 16) +
>                          (sensep[5] << 8) + sensep[6];
>         return (sensep[0] & 0x80) ? 1 : 0;
> 
> The latter implementation takes account of MMC devices.
> So I probably need to send a patch.

I can see that the newer version always stores an information value.  Are 
you suggesting that sr.c should ignore the return value and try to use 
info whenever it is nonzero and within the appropriate range?  Or should 
the decision to use info when the return value is 0 be based somehow on 
other information about the device?

Alan Stern

Re: [PATCH] Improve code for detecting errors near the end of a CD

[Douglas Gilbert]

Alan Stern wrote:
> On Thu, 20 Oct 2005, Douglas Gilbert wrote:
> 
> 
>>Alan,
>>Ouch.
>>Here is the linux kernel version (scsi_error.c):
>>
>>    case 0x70:
>>    case 0x71:
>>        if (sense_buffer[0] & 0x80) {
>>            *info_out = (sense_buffer[3] << 24) +
>>                        (sense_buffer[4] << 16) +
>>                        (sense_buffer[5] << 8) +
>>                        sense_buffer[6];
>>            return 1;
>>        } else
>>            return 0;
>>
>>and here is the sg3_utils version (sg_lib.c):
>>
>>    case 0x70:
>>    case 0x71:
>>        if (info_outp)
>>            *info_outp = (sensep[3] << 24) + (sensep[4] << 16) +
>>                         (sensep[5] << 8) + sensep[6];
>>        return (sensep[0] & 0x80) ? 1 : 0;
>>
>>The latter implementation takes account of MMC devices.
>>So I probably need to send a patch.
> 
> 
> I can see that the newer version always stores an information value.  Are 
> you suggesting that sr.c should ignore the return value and try to use 
> info whenever it is nonzero and within the appropriate range?  Or should 
> the decision to use info when the return value is 0 be based somehow on 
> other information about the device?

Alan,
The info field is optionally set for several SCSI
sense key values: medium and hardware error, perhaps
others. So if a ULD is invoking scsi_get_sense_info_fld()
I presume there is a good reason.

On disks I have seen the info field set to what looks
like the current lba when a REQUEST SENSE is sent
during a large transfer. The valid bit is clear
and the sense key is NO SENSE, so according to SBC
I regard that as a quaint response (but not an error).

The return value of scsi_get_sense_info_fld() should
be appropriately defined, that is it reflects whether
the "valid" bit is set.
I see the role of scsi_get_sense_info_fld() to yield as
much data as possible and let the ULD make the decision.
In the case of sr.c, it should be aware that due to
MMC not requiring the valid bit to be set, many
devices set the info field on a medium/hardware error
with the valid bit clear.

Note that for the "Information sense data descriptor"
(spc4r02 section 4.5.2.2) it is required that the valid
bit in there is set. Interestingly that was a change
introduced in spc3r22.pdf . So someone decided to stamp
on this fuzziness (when, in the future, devices switch
to descriptor sense format).

Doug Gilbert

[PATCH] Improve code for detecting errors near the end of a CD

[Alan Stern]

James:

This patch (as572) beefs up the code in sr.c that detects where read/write
errors occurred and moves it to a separate (but inline) routine for
greater clarity.

The original code contained one glaring mistake: testing
(SCpnt->sense_buffer[0] & 0x90) to see if the Information field in the
sense data is valid.  The Valid bit is 0x80; the test against 0x90 can
never fail, thanks to the earlier test: (SCpnt->sense_buffer[0] & 0x7f) ==
0x70).

The code also contained a rather puzzling line:

			error_sector &= ~(block_sectors - 1);

This makes no sense at all, so I have removed it.  The patch does of 
course check that error_sector is within the range of blocks that were 
supposed to be transferred.

The major enhancement is that the patch uses the Residue to calculate the 
error sector, if the sense data doesn't already provide it.  This helps 
with a drive I use.  Since the Residue isn't always reported correctly, 
the patch is careful to check that it has a reasonable value: somewhere 
strictly between 0 and the total transfer length.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Index: usb-2.6/drivers/scsi/sr.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sr.c
+++ usb-2.6/drivers/scsi/sr.c
@@ -203,7 +203,41 @@ int sr_media_change(struct cdrom_device_
 	}
 	return retval;
 }
- 
+
+/* 
+ * Use the information from a failed command to compute how much data
+ * was transferred successfully, and find the sector that caused the
+ * error.
+ */
+static inline int calc_good_bytes(struct scsi_cmnd *SCpnt,
+		long *error_sector, int blocksize)
+{
+	int num_bytes = SCpnt->bufflen;
+	unsigned int start_block, error_block, num_blocks = 0;
+
+	start_block = (unsigned int) SCpnt->request->sector / (blocksize >> 9);
+
+	if (SCpnt->sense_buffer[0] & 0x80) {
+		/* The Information field is valid; use it */
+		error_block = (SCpnt->sense_buffer[3] << 24) |
+				(SCpnt->sense_buffer[4] << 16) |
+				(SCpnt->sense_buffer[5] << 8) |
+				SCpnt->sense_buffer[6];
+		if (error_block >= start_block)
+			num_blocks = error_block - start_block;
+
+	} else {
+		/* Fall back on the Residue */
+		if (SCpnt->resid > 0 && SCpnt->resid < num_bytes)
+			num_blocks = (num_bytes - SCpnt->resid) / blocksize;
+	}
+
+	if (num_blocks >= num_bytes / blocksize)
+		num_blocks = 0;
+	*error_sector = (start_block + num_blocks) * (blocksize >> 9);
+	return num_blocks * blocksize;
+}
+
 /*
  * rw_intr is the interrupt routine for the device driver.
  *
@@ -235,25 +269,16 @@ static void rw_intr(struct scsi_cmnd * S
 		case MEDIUM_ERROR:
 		case VOLUME_OVERFLOW:
 		case ILLEGAL_REQUEST:
-			if (!(SCpnt->sense_buffer[0] & 0x90))
-				break;
 			if (!blk_fs_request(SCpnt->request))
 				break;
-			error_sector = (SCpnt->sense_buffer[3] << 24) |
-				(SCpnt->sense_buffer[4] << 16) |
-				(SCpnt->sense_buffer[5] << 8) |
-				SCpnt->sense_buffer[6];
 			if (SCpnt->request->bio != NULL)
 				block_sectors =
 					bio_sectors(SCpnt->request->bio);
 			if (block_sectors < 4)
 				block_sectors = 4;
-			if (cd->device->sector_size == 2048)
-				error_sector <<= 2;
-			error_sector &= ~(block_sectors - 1);
-			good_bytes = (error_sector - SCpnt->request->sector) << 9;
-			if (good_bytes < 0 || good_bytes >= this_count)
-				good_bytes = 0;
+
+			good_bytes = calc_good_bytes(SCpnt, &error_sector,
+					cd->device->sector_size);
 			/*
 			 * The SCSI specification allows for the value
 			 * returned by READ CAPACITY to be up to 75 2K