Re: [PATCH] normalize fixed and descriptor sense data

Re: [PATCH] normalize fixed and descriptor sense data

[Pat LaVarre]

Doug G:

 > so deferred errors can
 > be ignored in many contexts

Any hope of Linux always reporting that deferred errors have occurred, 
at least via dmesg if not in a critical dialog?

A deferred error, in the context of sense data ((bytes[0] & x7F) != 
x70) is usually a purportedly rare and therefore interesting 
catastrophe of some kind, yes?  A failure to write seen while flushing 
cache.  A failure to eject after obediently faking "immediate"ly good 
status.  Etc.

 > http://marc.theaimsgroup.com/?l=linux-scsi&m=109359716220586

Sorry I failed to click thru to the context for this thread.

Pat LaVarre

Re: [PATCH] normalize fixed and descriptor sense data

[Douglas Gilbert]

Pat LaVarre wrote:
> Doug G:
> 
>  > so deferred errors can
>  > be ignored in many contexts
> 
> Any hope of Linux always reporting that deferred errors have occurred, 
> at least via dmesg if not in a critical dialog?

I certainly hope so. One small change I made in my patch
yesterday was to pass deferred errors back to the block
SG_IO code.

This could be a largish job. First put a more robust
mechanism in place, then sweep the scsi mid level and
perhaps upper level drivers and convert them to the
new interface. The important thing to do is to think
about the error checking code distributed around the
place. It is reasonably easy to fix the wrong headed
stuff but wider issues are often involved (e.g. should
this be ignored and/or processed at a different level).

> A deferred error, in the context of sense data ((bytes[0] & x7F) != x70) 
> is usually a purportedly rare and therefore interesting catastrophe of 
> some kind, yes?  A failure to write seen while flushing cache.  A 
> failure to eject after obediently faking "immediate"ly good status.  Etc.
> 
>  > http://marc.theaimsgroup.com/?l=linux-scsi&m=109359716220586

Perhaps deferred errors should be picked up at the lowest
level of the midlevel, logged and ignored at higher levels
unless they are going back to a pass through interface.

> Sorry I failed to click thru to the context for this thread.

That is fine, the only other folks that I seemed to wake up
were the style police :-)

Doug Gilbert

Re: [PATCH] normalize fixed and descriptor sense data

[Kai Makisara]

On Sat, 28 Aug 2004, Douglas Gilbert wrote:

> Pat LaVarre wrote:
> > Doug G:
> > 
> >  > so deferred errors can
> >  > be ignored in many contexts
> > 
> > Any hope of Linux always reporting that deferred errors have occurred, 
> > at least via dmesg if not in a critical dialog?
> 
> I certainly hope so. One small change I made in my patch
> yesterday was to pass deferred errors back to the block
> SG_IO code.
> 
Block SG_IO seems to be a collection of serious bugs and/or
misfeatures ;-)

...
> > A deferred error, in the context of sense data ((bytes[0] & x7F) != x70) 
> > is usually a purportedly rare and therefore interesting catastrophe of 
> > some kind, yes?  A failure to write seen while flushing cache.  A 
> > failure to eject after obediently faking "immediate"ly good status.  Etc.
> > 
> >  > http://marc.theaimsgroup.com/?l=linux-scsi&m=109359716220586
> 
> Perhaps deferred errors should be picked up at the lowest
> level of the midlevel, logged and ignored at higher levels
> unless they are going back to a pass through interface.
> 
The severity of deferred errors depends on the device. The tapes seem to 
again be an exception from the mainstream. Streaming tape drives have to 
use largish internal buffers. This means that, for instance, write errors 
often are deferred errors. The possibilities to report this error to the 
user are limited but that is another thing (basically the driver can only 
suggest to the user that the tape head is in an uknown position).

So, I hope that there will be also in future a possibility for a driver to 
receive deferred errors and preferably without too much noise.

-- 
Kai

Re: [PATCH] normalize fixed and descriptor sense data

[Pat LaVarre]

Kai M:

>>> A deferred error, in the context of sense data ((bytes[0] & x7F) !=  
>>> x70)
>>> is usually a purportedly rare and therefore interesting catastrophe  
>>> of
>>> some kind, yes?  A failure to write seen while flushing cache.  A
>>> failure to eject after obediently faking "immediate"ly good status.   
>>> Etc.
> ...
> The severity of deferred errors depends on the device.
> ...
> Streaming tape drives have to
> use largish internal buffers. This means that, for instance, write  
> errors
> often are deferred errors. The possibilities to report this error to  
> the
> user are limited but that is another thing (basically the driver can  
> only
> suggest to the user that the tape head is in an uknown position).

Thanks for the tape perspective, most of my time I've spent with  
removable discs.

> The tapes seem to again be an exception from the mainstream.

I'm not sure I understand why?

Are we saying, for tape, a failure to write seen while flushing cache  
inside the device is normal and not catastrophic?  Are we saying tape  
buffers are commonly significantly larger than DVD-RAM buffers?  Help?

Pat LaVarre

P.S.

> Block SG_IO seems to be a collection of serious bugs and/or
> misfeatures ;-)

http://www.google.com/search?q=scsi+pass+thru+caveat
yields Doug G dated "04/09/2003":

---
The sg policy is basically "you want, you got it" **. Andre Hedrick  
found out that in some dark corner of the ATA instruction set it was  
possible to overspeed an ATA disk.  He proposed filtering out that  
instruction but the linux-kernel list consensus was not to do that.
---

grepping my own blogs for "caveat" yields:

--- Subject:  scsi pass thru en français, en español
--- http://plavarre.blog-city.com/read/618280.htm
...
Learning to use a tool like [SCSI pass thru] will probably remind you  
of the need to backup often. If you won't enjoy that lesson, then  
please don't pick up the tool. Potential side effects include kernel  
panics, destroyed discs, destroyed devices, and even imperceptibly  
subtle forms of data corruption. Storage devices differ from modems  
first by being designed to record your mistakes permanently, and second  
by defining some commands or sequences of commands to mean  
self-destruct.
...
---
--- Subject:  tools that demo the need to backup often
--- http://linux-pel.blog-city.com/read/546745.htm
... Solaris
... man uscsi
... http://www.google.com/search?q=solaris+man+uscsi
... http://www.sunspot.noao.edu/cgi-bin-local/man-cgi?uscsi+7I
... http://www.google.com/search?q=sys%2Fscsi%2Fimpl%2Fuscsi.h
...  
http://developers.sun.com/solaris/developer/support/driver/wps/uscsi/ 
uscsi.html
...
An unsupported, undocumented feature of Sun SCSI device drivers allows  
users to issue SCSI requests directly to SCSI devices. This capability  
is intended for driver prototyping and diagnostics; it allows the  
developer to try out sequences of SCSI commands without recompiling and  
reloading a device driver. Users of this feature must be forewarned  
that potential side effects include system panics and putting devices  
into unusable states.
...
---

-
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] normalize fixed and descriptor sense data

[Kai Makisara]

On Mon, 30 Aug 2004, Pat LaVarre wrote:

> Kai M:
> 
...
> > The severity of deferred errors depends on the device.
> > ...
> > Streaming tape drives have to
> > use largish internal buffers. This means that, for instance, write  
> > errors
> > often are deferred errors. The possibilities to report this error to  
> > the
> > user are limited but that is another thing (basically the driver can  
> > only
> > suggest to the user that the tape head is in an uknown position).
> 
> Thanks for the tape perspective, most of my time I've spent with  
> removable discs.
> 
> > The tapes seem to again be an exception from the mainstream.
> 
> I'm not sure I understand why?
> 
> Are we saying, for tape, a failure to write seen while flushing cache  
> inside the device is normal and not catastrophic?  Are we saying tape  
> buffers are commonly significantly larger than DVD-RAM buffers?  Help?
> 
The buffers in the faster drives may be quite big. For instance, I just 
looked at one LTO-2 drive specs and it had 64 MB buffer. However, this is 
not my point. If the error is not deferred, the driver can tell the user 
how much has been written but this does not help the user too much. When 
the error is signalled the user (using EIO), the user does not actually 
know if it is deferred or not. What you know is that the data up to the 
previous filemark is OK, beyond that the data is suspect unless the 
application knows the drive buffer size. To be able to easily read the 
archive, it is best to rewrite the archive on another tape. Bad tapes are 
not common but they do happen. That is why I say it is not a catastrophy 
to see a medium error of any type.

A good thing with tapes is that when the error is detected, the 
application is still living. If the last close succeeds, the data is on 
the tape. With disks it may be more complicated because there may be 
pending data from several sources and the application losing its data may 
not be around any more.

The severity of errors is largely a matter of opinion and depends on what 
you are trying to do. What we can probably agree on is that the scene 
looks a little different with different types of devices that SCSI can be  
used with. Besides disks and tapes, there are also other types (changers, 
backplanes, ...) that may need different type of processing. The high
level driver has to be able to make the decisions if it needs to. (Some 
decisions the upper level probably does not need to do like retrying and 
ABORTED command that has not done anything to the medium.)

Kai

Re: [PATCH] normalize fixed and descriptor sense data

[Pat LaVarre]

Kai M:

> ...
>>> The severity of deferred errors ...
> ...
> What you know is that the data up to the
> previous filemark is OK, beyond that the data is suspect unless the
> application knows the drive buffer size. To be able to easily read the
> archive, it is best to rewrite the archive on another tape. Bad tapes 
> are
> not common but they do happen. That is why I say it is not a 
> catastrophy
> to see a medium error of any type.

Ah.  I think I understand (& agree) now.

The "catastrophe" I meant was that the drive and media together have 
failed.  We now have cause to suspect both.

With removable media, yes, by design, after such an event, we recommend 
trust the drive, throw away the tape or disc, buy another, hope it 
works.  Of course, when the disk is not removable, then a failure of 
the disk alone means the drive loses all value.

> A good thing with tapes is that when the error is detected, the
> application is still living. If the last close succeeds, the data is on
> the tape. With disks it may be more complicated because there may be
> pending data from several sources and the application losing its data 
> may
> not be around any more.

"Ah.  I think I understand (& agree) now."

I distinguish drag & drop from backup.  With backup, to tape or disc, 
no matter.  Either way, the backup app should by design hang around 
long enough to see all trouble reported, and a report of bad media seen 
then means start over.

> The severity of errors is largely a matter of opinion and depends on 
> what
> you are trying to do. What we can probably agree on is that the scene
> looks a little different with different types of devices that SCSI can 
> be
> used with. Besides disks and tapes, there are also other types 
> (changers,
> backplanes, ...) that may need different type of processing.

Yes.

> The high
> level driver has to be able to make the decisions if it needs to. (Some
> decisions the upper level probably does not need to do

Yes.

> like retrying and
> ABORTED command that has not done anything to the medium.)

Some app folk want to decide their own retry strategy, aye.  General 
purpose operating systems easily fall into indefinite and pointless 
retries, aye.

Pat LaVarre

[PATCH] normalize fixed and descriptor sense data

[Douglas Gilbert]

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

This is a retransmission. My previous post on this
subject doesn't seem to have got through to the list.


After the comments on the RFC for this thread, here is
a patch against lk 2.6.9-rc1 to kick around.

The patch only touches two files: scsi.h and scsi_lib.c
It adds the proposed facility and then uses it in scsi_lib
in roughly 4 locations. IMO there were 3 sense processing errors:
    - block SG_IO did not get passed back deferred errors
      [SG_IO is a __pass-through__ interface!!]
    - MEDIUM_ERRORs _do_ get processed for deferred sense errors
      in scsi_io_completion() which seems unintended
      [I did not fix this one.]
    - invalid command operation code handling in
      __scsi_mode_sense() was just wrong

If people think this is a reasonable approach, then the rest of
the scsi mid-level and upper level driver could be converted.
As Kai pointed we may need some general routines to pick up the
sense data "extras".

The benefit of doing this conversion is that it may well
highlight a lot more sense data processing errors (if the
above is any guide).

Changes:
     - add structure to receive normalized sense data from either
       fixed or descriptor format sense data
     - add scsi_normalize_sense() function to populate that structure
     - add scsi_sense_is_deferred() function so deferred errors can
       be ignored in many contexts
     - apply new mechanism to sense data processing within the
       scsi_lib.c file

Doug Gilbert


[-- Attachment #2: scsi_sdesc269rc1.diff --]
[-- Type: text/x-patch, Size: 6281 bytes --]

--- linux/include/scsi/scsi.h	2004-08-25 10:06:42.000000000 +1000
+++ linux/include/scsi/scsi.h269rc1sdesc	2004-08-27 14:35:42.000000000 +1000
@@ -236,6 +236,33 @@
 	__u8 scsi_lun[8];
 };
 
+/* This is a slightly modified SCSI sense "descriptor" format header.
+ * The addition is to allow the 0x70 and 0x71 response codes. The idea
+ * is to place the salient data from either "fixed" or "descriptor" sense
+ * format into one structure to ease application processing.
+ * The original sense buffer should be kept around for those cases
+ * in which more information is required (e.g. the LBA of a MEDIUM ERROR).
+ */
+struct scsi_sense_hd {     /* See SPC-3 section 4.5 */
+    uint8_t response_code; /* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
+    uint8_t sense_key;
+    uint8_t asc;
+    uint8_t ascq;
+    uint8_t byte4;
+    uint8_t byte5;
+    uint8_t byte6;
+    uint8_t additional_length; /* always 0 for fixed sense format */
+};
+
+extern int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
+                                struct scsi_sense_hd * sshd);
+
+static inline int scsi_sense_is_deferred(struct scsi_sense_hd * sshd)
+{
+	return (sshd && (sshd->response_code >= 0x70) && 
+		(sshd->response_code & 1)) ? 1 : 0;
+}
+ 
 /*
  *  MESSAGE CODES
  */
--- linux/drivers/scsi/scsi_lib.c	2004-08-25 10:06:39.000000000 +1000
+++ linux/drivers/scsi/scsi_lib.c269rc1sdesc	2004-08-27 15:38:03.000000000 +1000
@@ -692,6 +692,7 @@
 	request_queue_t *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
+	struct scsi_sense_hd sshd;
 
 	/*
 	 * Free up any indirection buffers we allocated for DMA purposes. 
@@ -715,7 +716,9 @@
 			      (CHECK_CONDITION << 1) : (result & 0xff);
 		if (result) {
 			clear_errors = 0;
-			if (cmd->sense_buffer[0] & 0x70) {
+			if (scsi_normalize_sense(cmd->sense_buffer,
+					sizeof cmd->sense_buffer, &sshd)) {
+				/* SG_IO wants to know about deferred errors */
 				int len = 8 + cmd->sense_buffer[7];
 
 				if (len > SCSI_SENSE_BUFFERSIZE)
@@ -774,17 +777,18 @@
 	 * can choose a block to remap, etc.
 	 */
 	if (driver_byte(result) != 0) {
-		if ((cmd->sense_buffer[0] & 0x7f) == 0x70) {
+		if (scsi_normalize_sense(cmd->sense_buffer,
+					 SCSI_SENSE_BUFFERSIZE, &sshd) &&
+		    (! scsi_sense_is_deferred(&sshd))) {
 			/*
 			 * If the device is in the process of becoming ready,
 			 * retry.
 			 */
-			if (cmd->sense_buffer[12] == 0x04 &&
-			    cmd->sense_buffer[13] == 0x01) {
+			if (sshd.asc == 0x04 && sshd.ascq == 0x01) {
 				scsi_requeue_command(q, cmd);
 				return;
 			}
-			if ((cmd->sense_buffer[2] & 0xf) == UNIT_ATTENTION) {
+			if (sshd.sense_key == UNIT_ATTENTION) {
 				if (cmd->device->removable) {
 					/* detected disc change.  set a bit 
 					 * and quietly refuse further access.
@@ -813,7 +817,9 @@
 		 * failed, we may have read past the end of the disk.
 		 */
 
-		switch (cmd->sense_buffer[2]) {
+/* Following is broken since deferred errors fall through. [dpg 20040827] */
+
+		switch (sshd.sense_key) {
 		case ILLEGAL_REQUEST:
 			if (cmd->device->use_10_for_rw &&
 			    (cmd->cmnd[0] == READ_10 ||
@@ -1496,8 +1502,7 @@
 	}
 
 	sreq->sr_cmd_len = 0;
-	sreq->sr_sense_buffer[0] = 0;
-	sreq->sr_sense_buffer[2] = 0;
+	memset(sreq->sr_sense_buffer, 0, sizeof(sreq->sr_sense_buffer));
 	sreq->sr_data_direction = DMA_FROM_DEVICE;
 
 	memset(buffer, 0, len);
@@ -1508,14 +1513,20 @@
 	 * ILLEGAL REQUEST sense return identifies the actual command
 	 * byte as the problem.  MODE_SENSE commands can return
 	 * ILLEGAL REQUEST if the code page isn't supported */
-	if (use_10_for_ms && ! scsi_status_is_good(sreq->sr_result) &&
-	    (driver_byte(sreq->sr_result) & DRIVER_SENSE) &&
-	    sreq->sr_sense_buffer[2] == ILLEGAL_REQUEST &&
-	    (sreq->sr_sense_buffer[4] & 0x40) == 0x40 &&
-	    sreq->sr_sense_buffer[5] == 0 &&
-	    sreq->sr_sense_buffer[6] == 0 ) {
-		sreq->sr_device->use_10_for_ms = 0;
-		goto retry;
+
+	if (use_10_for_ms && (! scsi_status_is_good(sreq->sr_result)) &&
+	    (driver_byte(sreq->sr_result) & DRIVER_SENSE)) {
+		struct scsi_sense_hd sshd;
+
+		if (scsi_normalize_sense(sreq->sr_sense_buffer,
+			 sizeof sreq->sr_sense_buffer, &sshd)) {
+			if ((sshd.sense_key == ILLEGAL_REQUEST) &&
+			    (sshd.asc == 0x20)) {
+				/* Invalid command operation code */
+				sreq->sr_device->use_10_for_ms = 0;
+				goto retry;
+			}
+		}
 	}
 
 	if(scsi_status_is_good(sreq->sr_result)) {
@@ -1711,3 +1722,55 @@
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+/**
+ *	scsi_normalize_sense - normalize main elements from either fixed
+ *		or descriptor sense data format into a common format.
+ *	@sense_buffer:	byte array containing sense data returned by
+ *			device
+ *	@sb_len:	number of valid bytes in sense_buffer
+ *	@sshd:		pointer to instance of structure that common
+ *			elements are written to. Ignored if NULL.
+ *
+ *	The "main elements" from sense data are: response_code, sense_key,
+ *	asc, ascq and additional_length (only for descriptor format).
+ *	Typically this function can be called after a device has
+ *	responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ *	Returns 1 if valid sense data information found, else 0;
+ **/
+int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
+                         struct scsi_sense_hd * sshd)
+{
+	if (sshd)
+		memset(sshd, 0, sizeof(struct scsi_sense_hd));
+	if ((NULL == sense_buffer) || (0 == sb_len) ||
+	    (0x70 != (0x70 & sense_buffer[0])))
+		return 0;
+	if (NULL == sshd)
+		return 1;
+	sshd->response_code = (0x7f & sense_buffer[0]);
+	if (sshd->response_code >= 0x72) {	/* descriptor format */
+		if (sb_len > 1)
+			sshd->sense_key = (0xf & sense_buffer[1]);
+		if (sb_len > 2)
+			sshd->asc = sense_buffer[2];
+		if (sb_len > 3)
+			sshd->ascq = sense_buffer[3];
+		if (sb_len > 7)
+			sshd->additional_length = sense_buffer[7];
+	} else {				/* fixed format */
+		if (sb_len > 2)
+			sshd->sense_key = (0xf & sense_buffer[2]);
+		if (sb_len > 7) {
+			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
+					 sb_len : (sense_buffer[7] + 8);
+			if (sb_len > 12)
+				sshd->asc = sense_buffer[12];
+			if (sb_len > 13)
+				sshd->ascq = sense_buffer[13];
+		}
+	}
+	return 1;
+}
+EXPORT_SYMBOL(scsi_normalize_sense);
+

Re: [PATCH] normalize fixed and descriptor sense data

[Christoph Hellwig]

+struct scsi_sense_hd {     /* See SPC-3 section 4.5 */

maybe scsi_sense_desc insted?

+    uint8_t response_code; /* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
+    uint8_t sense_key;
+    uint8_t asc;
+    uint8_t ascq;
+    uint8_t byte4;
+    uint8_t byte5;
+    uint8_t byte6;
+    uint8_t additional_length; /* always 0 for fixed sense format */

We use u8 elsewhere in the scsi code, please stay consistant.

+static inline int scsi_sense_is_deferred(struct scsi_sense_hd * sshd)
+{
+	return (sshd && (sshd->response_code >= 0x70) && 
+		(sshd->response_code & 1)) ? 1 : 0;
+}

I don't think we need the NULL check for sshd here, do we?

+/**
+ *	scsi_normalize_sense - normalize main elements from either fixed
+ *		or descriptor sense data format into a common format.
+ *	@sense_buffer:	byte array containing sense data returned by
+ *			device
+ *	@sb_len:	number of valid bytes in sense_buffer
+ *	@sshd:		pointer to instance of structure that common
+ *			elements are written to. Ignored if NULL.
+ *
+ *	The "main elements" from sense data are: response_code, sense_key,
+ *	asc, ascq and additional_length (only for descriptor format).
+ *	Typically this function can be called after a device has
+ *	responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ *	Returns 1 if valid sense data information found, else 0;
+ **/
+int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
+                         struct scsi_sense_hd * sshd)
+{
+	if (sshd)
+		memset(sshd, 0, sizeof(struct scsi_sense_hd));

Again, checking for a NULL sshd here doesn't make much sense.

Also I think this function should get a __ prefix and we should
add two small helpers that take a scsi_cmnd/scsi_request and use
the sense_buffer and len from it.

+	if ((NULL == sense_buffer) || (0 == sb_len) ||
+	    (0x70 != (0x70 & sense_buffer[0])))
+		return 0;
+	if (NULL == sshd)
+		return 1;

Wrong way around vs normal scsi style.  Should be more like:

	if (!sense_buffer || !sb_len || (0x70 & sense_buffer[0]) != 0x70)
		return 0;

(and the sshd check gone again)
(same issues come up below again a few times)

Re: [PATCH] normalize fixed and descriptor sense data

[Luben Tuikov]

Christoph Hellwig wrote:
> 
> +struct scsi_sense_hd {     /* See SPC-3 section 4.5 */
> 
> maybe scsi_sense_desc insted?

Why, when it's neither in descriptor format nor in fixed
format?

It is just a header of the _consolidated_ format
of the sense data, thus the name used makes sense.
 
> +    uint8_t response_code; /* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
> +    uint8_t sense_key;
> +    uint8_t asc;
> +    uint8_t ascq;
> +    uint8_t byte4;
> +    uint8_t byte5;
> +    uint8_t byte6;
> +    uint8_t additional_length; /* always 0 for fixed sense format */
> 
> We use u8 elsewhere in the scsi code, please stay consistant.
> 
> +static inline int scsi_sense_is_deferred(struct scsi_sense_hd * sshd)
> +{
> +       return (sshd && (sshd->response_code >= 0x70) &&
> +               (sshd->response_code & 1)) ? 1 : 0;
> +}
> 
> I don't think we need the NULL check for sshd here, do we?
> 
> +/**
> + *     scsi_normalize_sense - normalize main elements from either fixed
> + *             or descriptor sense data format into a common format.
> + *     @sense_buffer:  byte array containing sense data returned by
> + *                     device
> + *     @sb_len:        number of valid bytes in sense_buffer
> + *     @sshd:          pointer to instance of structure that common
> + *                     elements are written to. Ignored if NULL.
> + *
> + *     The "main elements" from sense data are: response_code, sense_key,
> + *     asc, ascq and additional_length (only for descriptor format).
> + *     Typically this function can be called after a device has
> + *     responded to a SCSI command with the CHECK_CONDITION status.
> + *
> + *     Returns 1 if valid sense data information found, else 0;
> + **/
> +int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
> +                         struct scsi_sense_hd * sshd)
> +{
> +       if (sshd)
> +               memset(sshd, 0, sizeof(struct scsi_sense_hd));
> 
> Again, checking for a NULL sshd here doesn't make much sense.
> 
> Also I think this function should get a __ prefix and we should
> add two small helpers that take a scsi_cmnd/scsi_request and use
> the sense_buffer and len from it.

But it should be exported nevertheless, as LLDDs may like
to use it.

		Luben

 
> +       if ((NULL == sense_buffer) || (0 == sb_len) ||
> +           (0x70 != (0x70 & sense_buffer[0])))
> +               return 0;
> +       if (NULL == sshd)
> +               return 1;
> 
> Wrong way around vs normal scsi style.  Should be more like:
> 
>         if (!sense_buffer || !sb_len || (0x70 & sense_buffer[0]) != 0x70)
>                 return 0;
> 
> (and the sshd check gone again)
> (same issues come up below again a few times)
> 
> -
> 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] normalize fixed and descriptor sense data

[Christoph Hellwig]

On Fri, Aug 27, 2004 at 11:45:30AM -0400, Luben Tuikov wrote:
> Christoph Hellwig wrote:
> > 
> > +struct scsi_sense_hd {     /* See SPC-3 section 4.5 */
> > 
> > maybe scsi_sense_desc insted?
> 
> Why, when it's neither in descriptor format nor in fixed
> format?
> 
> It is just a header of the _consolidated_ format
> of the sense data, thus the name used makes sense.

Umm, right, makes sense.  the _hd postfix still looks strange to me.
Either _hdr as standard short-notation for header or even better just
remove it.

> > Also I think this function should get a __ prefix and we should
> > add two small helpers that take a scsi_cmnd/scsi_request and use
> > the sense_buffer and len from it.
> 
> But it should be exported nevertheless, as LLDDs may like
> to use it.

Makes sense.

Re: [PATCH] normalize fixed and descriptor sense data

[Randy.Dunlap]

On Fri, 27 Aug 2004 18:56:42 +1000 Douglas Gilbert wrote:

| After the comments on the RFC for this thread, here is
| a patch against lk 2.6.9-rc1 to kick around.
| 

A few more style comments to go along with Christoph's comments.

a.  Linux long comment style begins with "/*" on a line by itself.

b.  sizeof xyz:  style is usually sizeof(xyz)

[I find 440 of:		sizeof xyz
	2106 of:	sizeof (xyz)
	26948 of:	sizeof(xyz)
in 2.6.8.1]

c.  +		    (! scsi_sense_is_deferred(&sshd))) {

Drop the space after '!'  [from someone who loves to use spaces :]


--
~Randy

Re: [PATCH] normalize fixed and descriptor sense data

[Douglas Gilbert]

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

Douglas Gilbert wrote:
> After the comments on the RFC for this thread, here is
> a patch against lk 2.6.9-rc1 to kick around.
> 
> The patch only touches two files: scsi.h and scsi_lib.c
> It adds the proposed facility and then uses it in scsi_lib
> in roughly 4 locations. IMO there were 3 sense processing errors:
>    - block SG_IO did not get passed back deferred errors
>      [SG_IO is a __pass-through__ interface!!]
>    - MEDIUM_ERRORs _do_ get processed for deferred sense errors
>      in scsi_io_completion() which seems unintended
>      [I did not fix this one.]
>    - invalid command operation code handling in
>      __scsi_mode_sense() was just wrong
> 
> If people think this is a reasonable approach, then the rest of
> the scsi mid-level and upper level driver could be converted.
> As Kai pointed we may need some general routines to pick up the
> sense data "extras".
> 
> The benefit of doing this conversion is that it may well
> highlight a lot more sense data processing errors (if the
> above is any guide).
> 
> Changes:
>     - add structure to receive normalized sense data from either
>       fixed or descriptor format sense data
>     - add scsi_normalize_sense() function to populate that structure
>     - add scsi_sense_is_deferred() function so deferred errors can
>       be ignored in many contexts
>     - apply new mechanism to sense data processing within the
>       scsi_lib.c file

Here is a second cut of the patch.

If James accepts it naturally he is free to enforce the
appropriate coding style.

I believe that cleaning up scsi error processing is going
to be a "work in progress" for a while.

Doug Gilbert

[-- Attachment #2: scsi_sdesc269rc1_2.diff --]
[-- Type: text/x-patch, Size: 6687 bytes --]

--- linux/include/scsi/scsi.h	2004-08-25 10:06:42.000000000 +1000
+++ linux/include/scsi/scsi.h269rc1sdesc2	2004-08-28 14:17:48.000000000 +1000
@@ -236,6 +236,33 @@
 	__u8 scsi_lun[8];
 };
 
+/* This is a slightly modified SCSI sense "descriptor" format header.
+ * The addition is to allow the 0x70 and 0x71 response codes. The idea
+ * is to place the salient data from either "fixed" or "descriptor" sense
+ * format into one structure to ease application processing.
+ * The original sense buffer should be kept around for those cases
+ * in which more information is required (e.g. the LBA of a MEDIUM ERROR).
+ */
+struct scsi_sense_hdr {	   /* See SPC-3 section 4.5 */
+    uint8_t response_code; /* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
+    uint8_t sense_key;
+    uint8_t asc;
+    uint8_t ascq;
+    uint8_t byte4;
+    uint8_t byte5;
+    uint8_t byte6;
+    uint8_t additional_length; /* always 0 for fixed sense format */
+};
+
+extern int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
+                                struct scsi_sense_hdr * sshdr);
+
+static inline int scsi_sense_is_deferred(struct scsi_sense_hdr * sshdr)
+{
+	return ((sshdr->response_code >= 0x70) && (sshdr->response_code & 1))
+	       ? 1 : 0;
+}
+ 
 /*
  *  MESSAGE CODES
  */
--- linux/drivers/scsi/scsi_lib.c	2004-08-25 10:06:39.000000000 +1000
+++ linux/drivers/scsi/scsi_lib.c269rc1sdesc2	2004-08-28 14:48:40.000000000 +1000
@@ -692,6 +692,7 @@
 	request_queue_t *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
+	struct scsi_sense_hdr sshdr;
 
 	/*
 	 * Free up any indirection buffers we allocated for DMA purposes. 
@@ -715,7 +716,11 @@
 			      (CHECK_CONDITION << 1) : (result & 0xff);
 		if (result) {
 			clear_errors = 0;
-			if (cmd->sense_buffer[0] & 0x70) {
+			if (scsi_normalize_sense(cmd->sense_buffer,
+					sizeof(cmd->sense_buffer), &sshdr)) {
+				/*
+				 * SG_IO wants to know about deferred errors
+				 */
 				int len = 8 + cmd->sense_buffer[7];
 
 				if (len > SCSI_SENSE_BUFFERSIZE)
@@ -774,17 +779,18 @@
 	 * can choose a block to remap, etc.
 	 */
 	if (driver_byte(result) != 0) {
-		if ((cmd->sense_buffer[0] & 0x7f) == 0x70) {
+		if (scsi_normalize_sense(cmd->sense_buffer,
+					 SCSI_SENSE_BUFFERSIZE, &sshdr) &&
+		    (! scsi_sense_is_deferred(&sshdr))) {
 			/*
 			 * If the device is in the process of becoming ready,
 			 * retry.
 			 */
-			if (cmd->sense_buffer[12] == 0x04 &&
-			    cmd->sense_buffer[13] == 0x01) {
+			if (sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
 				scsi_requeue_command(q, cmd);
 				return;
 			}
-			if ((cmd->sense_buffer[2] & 0xf) == UNIT_ATTENTION) {
+			if (sshdr.sense_key == UNIT_ATTENTION) {
 				if (cmd->device->removable) {
 					/* detected disc change.  set a bit 
 					 * and quietly refuse further access.
@@ -813,7 +819,10 @@
 		 * failed, we may have read past the end of the disk.
 		 */
 
-		switch (cmd->sense_buffer[2]) {
+/*
+ * Following is probably broken since deferred errors fall through [dpg 20040827]
+ */
+		switch (sshdr.sense_key) {
 		case ILLEGAL_REQUEST:
 			if (cmd->device->use_10_for_rw &&
 			    (cmd->cmnd[0] == READ_10 ||
@@ -835,8 +844,6 @@
 			       req->rq_disk ? req->rq_disk->disk_name : "");
 			cmd = scsi_end_request(cmd, 0, this_count, 1);
 			return;
-			break;
-		case MEDIUM_ERROR:
 		case VOLUME_OVERFLOW:
 			printk("scsi%d: ERROR on channel %d, id %d, lun %d, CDB: ",
 			       cmd->device->host->host_no, (int) cmd->device->channel,
@@ -1496,8 +1503,7 @@
 	}
 
 	sreq->sr_cmd_len = 0;
-	sreq->sr_sense_buffer[0] = 0;
-	sreq->sr_sense_buffer[2] = 0;
+	memset(sreq->sr_sense_buffer, 0, sizeof(sreq->sr_sense_buffer));
 	sreq->sr_data_direction = DMA_FROM_DEVICE;
 
 	memset(buffer, 0, len);
@@ -1508,14 +1514,22 @@
 	 * ILLEGAL REQUEST sense return identifies the actual command
 	 * byte as the problem.  MODE_SENSE commands can return
 	 * ILLEGAL REQUEST if the code page isn't supported */
-	if (use_10_for_ms && ! scsi_status_is_good(sreq->sr_result) &&
-	    (driver_byte(sreq->sr_result) & DRIVER_SENSE) &&
-	    sreq->sr_sense_buffer[2] == ILLEGAL_REQUEST &&
-	    (sreq->sr_sense_buffer[4] & 0x40) == 0x40 &&
-	    sreq->sr_sense_buffer[5] == 0 &&
-	    sreq->sr_sense_buffer[6] == 0 ) {
-		sreq->sr_device->use_10_for_ms = 0;
-		goto retry;
+
+	if (use_10_for_ms && (! scsi_status_is_good(sreq->sr_result)) &&
+	    (driver_byte(sreq->sr_result) & DRIVER_SENSE)) {
+		struct scsi_sense_hdr sshdr;
+
+		if (scsi_normalize_sense(sreq->sr_sense_buffer,
+			 sizeof(sreq->sr_sense_buffer), &sshdr)) {
+			if ((sshdr.sense_key == ILLEGAL_REQUEST) &&
+			    (sshdr.asc == 0x20) && (sshdr.ascq == 0)) {
+				/* 
+				 * Invalid command operation code
+				 */
+				sreq->sr_device->use_10_for_ms = 0;
+				goto retry;
+			}
+		}
 	}
 
 	if(scsi_status_is_good(sreq->sr_result)) {
@@ -1711,3 +1725,58 @@
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+/**
+ *	scsi_normalize_sense - normalize main elements from either fixed
+ *		or descriptor sense data format into a common format.
+ *	@sense_buffer:	byte array containing sense data returned by
+ *			device
+ *	@sb_len:	number of valid bytes in sense_buffer
+ *	@sshdr:		pointer to instance of structure that common
+ *			elements are written to. Ignored if NULL.
+ *
+ *	The "main elements" from sense data are: response_code, sense_key,
+ *	asc, ascq and additional_length (only for descriptor format).
+ *	Typically this function can be called after a device has
+ *	responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ *	Returns 1 if valid sense data information found, else 0;
+ **/
+int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
+                         struct scsi_sense_hdr * sshdr)
+{
+	memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+	if ((sense_buffer == NULL) || (sb_len == 0) ||
+	    ((0x70 & sense_buffer[0]) != 0x70))
+		return 0;
+	sshdr->response_code = (sense_buffer[0] & 0x7f);
+	if (sshdr->response_code >= 0x72) {
+		/*
+		 * descriptor format
+		 */
+		if (sb_len > 1)
+			sshdr->sense_key = (sense_buffer[1] & 0xf);
+		if (sb_len > 2)
+			sshdr->asc = sense_buffer[2];
+		if (sb_len > 3)
+			sshdr->ascq = sense_buffer[3];
+		if (sb_len > 7)
+			sshdr->additional_length = sense_buffer[7];
+	} else {
+		/* 
+		 * fixed format
+		 */
+		if (sb_len > 2)
+			sshdr->sense_key = (sense_buffer[2] & 0xf);
+		if (sb_len > 7) {
+			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
+					 sb_len : (sense_buffer[7] + 8);
+			if (sb_len > 12)
+				sshdr->asc = sense_buffer[12];
+			if (sb_len > 13)
+				sshdr->ascq = sense_buffer[13];
+		}
+	}
+	return 1;
+}
+EXPORT_SYMBOL(scsi_normalize_sense);
+

Re: [PATCH] normalize fixed and descriptor sense data

[Christoph Hellwig]

On Sat, Aug 28, 2004 at 03:05:35PM +1000, Douglas Gilbert wrote:
> Here is a second cut of the patch.
> 
> If James accepts it naturally he is free to enforce the
> appropriate coding style.
> 
> I believe that cleaning up scsi error processing is going
> to be a "work in progress" for a while.

Here's a version with a few small adjustments:

 - moved the code to the scsi_error.c/scsi_eh.c
 - small style cleanups
 - added the two helpers I suggested
 - don't do the memset if we return failure, callers shouldn't look at
   it then

--- linux-2.6.9-rc1-mm1/drivers/scsi/scsi_error.c	2004-08-28 12:17:33.000000000 +0200
+++ linux-2.6.9-rc1-mm1-hch/drivers/scsi/scsi_error.c	2004-08-28 13:20:30.000000000 +0200
@@ -1850,3 +1850,79 @@
 	scsi_next_command(scmd);
 	return rtn;
 }
+
+/**
+ * scsi_normalize_sense - normalize main elements from either fixed or
+ *			descriptor sense data format into a common format.
+ *
+ * @sense_buffer:	byte array containing sense data returned by device
+ * @sb_len:		number of valid bytes in sense_buffer
+ * @sshdr:		pointer to instance of structure that common
+ *			elements are written to. Ignored if NULL.
+ *
+ * Notes:
+ *	The "main elements" from sense data are: response_code, sense_key,
+ *	asc, ascq and additional_length (only for descriptor format).
+ *
+ *	Typically this function can be called after a device has
+ *	responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ * Return value:
+ *	1 if valid sense data information found, else 0;
+ **/
+int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
+                         struct scsi_sense_hdr *sshdr)
+{
+	if (!sense_buffer || !sb_len || (sense_buffer[0] & 0x70) != 0x70)
+		return 0;
+
+	memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
+	sshdr->response_code = (sense_buffer[0] & 0x7f);
+	if (sshdr->response_code >= 0x72) {
+		/*
+		 * descriptor format
+		 */
+		if (sb_len > 1)
+			sshdr->sense_key = (sense_buffer[1] & 0xf);
+		if (sb_len > 2)
+			sshdr->asc = sense_buffer[2];
+		if (sb_len > 3)
+			sshdr->ascq = sense_buffer[3];
+		if (sb_len > 7)
+			sshdr->additional_length = sense_buffer[7];
+	} else {
+		/* 
+		 * fixed format
+		 */
+		if (sb_len > 2)
+			sshdr->sense_key = (sense_buffer[2] & 0xf);
+		if (sb_len > 7) {
+			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
+					 sb_len : (sense_buffer[7] + 8);
+			if (sb_len > 12)
+				sshdr->asc = sense_buffer[12];
+			if (sb_len > 13)
+				sshdr->ascq = sense_buffer[13];
+		}
+	}
+
+	return 1;
+}
+EXPORT_SYMBOL(scsi_normalize_sense);
+
+int scsi_request_normalize_sense(struct scsi_request *sreq,
+				 struct scsi_sense_hdr *sshdr)
+{
+	return scsi_normalize_sense(sreq->sr_sense_buffer,
+			sizeof(sreq->sr_sense_buffer), sshdr);
+}
+EXPORT_SYMBOL(scsi_request_normalize_sense);
+
+int scsi_command_normalize_sense(struct scsi_cmnd *cmd,
+				 struct scsi_sense_hdr *sshdr)
+{
+	return scsi_normalize_sense(cmd->sense_buffer,
+			sizeof(cmd->sense_buffer), sshdr);
+}
+EXPORT_SYMBOL(scsi_command_normalize_sense);
--- linux-2.6.9-rc1-mm1/drivers/scsi/scsi_lib.c	2004-08-27 16:41:19.000000000 +0200
+++ linux-2.6.9-rc1-mm1-hch/drivers/scsi/scsi_lib.c	2004-08-28 12:51:09.000000000 +0200
@@ -692,6 +692,7 @@
 	request_queue_t *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
+	struct scsi_sense_hdr sshdr;
 
 	/*
 	 * Free up any indirection buffers we allocated for DMA purposes. 
@@ -715,7 +716,10 @@
 			      (CHECK_CONDITION << 1) : (result & 0xff);
 		if (result) {
 			clear_errors = 0;
-			if (cmd->sense_buffer[0] & 0x70) {
+			if (scsi_command_normalize_sense(cmd, &sshdr)) {
+				/*
+				 * SG_IO wants to know about deferred errors
+				 */
 				int len = 8 + cmd->sense_buffer[7];
 
 				if (len > SCSI_SENSE_BUFFERSIZE)
@@ -774,17 +778,17 @@
 	 * can choose a block to remap, etc.
 	 */
 	if (driver_byte(result) != 0) {
-		if ((cmd->sense_buffer[0] & 0x7f) == 0x70) {
+		if (scsi_command_normalize_sense(cmd, &sshdr) &&
+				!scsi_sense_is_deferred(&sshdr)) {
 			/*
 			 * If the device is in the process of becoming ready,
 			 * retry.
 			 */
-			if (cmd->sense_buffer[12] == 0x04 &&
-			    cmd->sense_buffer[13] == 0x01) {
+			if (sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
 				scsi_requeue_command(q, cmd);
 				return;
 			}
-			if ((cmd->sense_buffer[2] & 0xf) == UNIT_ATTENTION) {
+			if (sshdr.sense_key == UNIT_ATTENTION) {
 				if (cmd->device->removable) {
 					/* detected disc change.  set a bit 
 					 * and quietly refuse further access.
@@ -813,7 +817,11 @@
 		 * failed, we may have read past the end of the disk.
 		 */
 
-		switch (cmd->sense_buffer[2]) {
+		/*
+		 * XXX: Following is probably broken since deferred errors
+		 *	fall through [dpg 20040827]
+		 */
+		switch (sshdr.sense_key) {
 		case ILLEGAL_REQUEST:
 			if (cmd->device->use_10_for_rw &&
 			    (cmd->cmnd[0] == READ_10 ||
@@ -835,8 +843,6 @@
 			       req->rq_disk ? req->rq_disk->disk_name : "");
 			cmd = scsi_end_request(cmd, 0, this_count, 1);
 			return;
-			break;
-		case MEDIUM_ERROR:
 		case VOLUME_OVERFLOW:
 			printk("scsi%d: ERROR on channel %d, id %d, lun %d, CDB: ",
 			       cmd->device->host->host_no, (int) cmd->device->channel,
@@ -1496,8 +1502,7 @@
 	}
 
 	sreq->sr_cmd_len = 0;
-	sreq->sr_sense_buffer[0] = 0;
-	sreq->sr_sense_buffer[2] = 0;
+	memset(sreq->sr_sense_buffer, 0, sizeof(sreq->sr_sense_buffer));
 	sreq->sr_data_direction = DMA_FROM_DEVICE;
 
 	memset(buffer, 0, len);
@@ -1508,14 +1513,21 @@
 	 * ILLEGAL REQUEST sense return identifies the actual command
 	 * byte as the problem.  MODE_SENSE commands can return
 	 * ILLEGAL REQUEST if the code page isn't supported */
-	if (use_10_for_ms && ! scsi_status_is_good(sreq->sr_result) &&
-	    (driver_byte(sreq->sr_result) & DRIVER_SENSE) &&
-	    sreq->sr_sense_buffer[2] == ILLEGAL_REQUEST &&
-	    (sreq->sr_sense_buffer[4] & 0x40) == 0x40 &&
-	    sreq->sr_sense_buffer[5] == 0 &&
-	    sreq->sr_sense_buffer[6] == 0 ) {
-		sreq->sr_device->use_10_for_ms = 0;
-		goto retry;
+
+	if (use_10_for_ms && !scsi_status_is_good(sreq->sr_result) &&
+	    (driver_byte(sreq->sr_result) & DRIVER_SENSE)) {
+		struct scsi_sense_hdr sshdr;
+
+		if (scsi_request_normalize_sense(sreq, &sshdr)) {
+			if ((sshdr.sense_key == ILLEGAL_REQUEST) &&
+			    (sshdr.asc == 0x20) && (sshdr.ascq == 0)) {
+				/* 
+				 * Invalid command operation code
+				 */
+				sreq->sr_device->use_10_for_ms = 0;
+				goto retry;
+			}
+		}
 	}
 
 	if(scsi_status_is_good(sreq->sr_result)) {
@@ -1710,4 +1722,3 @@
 	scsi_run_queue(sdev->request_queue);
 }
 EXPORT_SYMBOL(scsi_device_resume);
-
--- linux-2.6.9-rc1-mm1/include/scsi/scsi_eh.h	2004-08-28 12:17:39.000000000 +0200
+++ linux-2.6.9-rc1-mm1-hch/include/scsi/scsi_eh.h	2004-08-28 14:36:26.000000000 +0200
@@ -3,15 +3,48 @@
 
 struct scsi_cmnd;
 struct scsi_device;
+struct scsi_request;
 struct Scsi_Host;
 
+/*
+ * This is a slightly modified SCSI sense "descriptor" format header.
+ * The addition is to allow the 0x70 and 0x71 response codes. The idea
+ * is to place the salient data from either "fixed" or "descriptor" sense
+ * format into one structure to ease application processing.
+ *
+ * The original sense buffer should be kept around for those cases
+ * in which more information is required (e.g. the LBA of a MEDIUM ERROR).
+ */
+struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
+	u8 response_code;	/* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
+	u8 sense_key;
+	u8 asc;
+	u8 ascq;
+	u8 byte4;
+	u8 byte5;
+	u8 byte6;
+	u8 additional_length;	/* always 0 for fixed sense format */
+};
+
+
 extern void scsi_add_timer(struct scsi_cmnd *, int,
-			   void (*)(struct scsi_cmnd *));
+		void (*)(struct scsi_cmnd *));
 extern int scsi_delete_timer(struct scsi_cmnd *);
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
 extern int scsi_block_when_processing_errors(struct scsi_device *);
+extern int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
+		struct scsi_sense_hdr *sshdr);
+extern int scsi_request_normalize_sense(struct scsi_request *sreq,
+		struct scsi_sense_hdr *sshdr);
+extern int scsi_command_normalize_sense(struct scsi_cmnd *cmd,
+		struct scsi_sense_hdr *sshdr);
 
+static inline int scsi_sense_is_deferred(struct scsi_sense_hdr *sshdr)
+{
+	return ((sshdr->response_code >= 0x70) && (sshdr->response_code & 1));
+}
+ 
 /*
  * Reset request from external source
  */

Re: [PATCH] normalize fixed and descriptor sense data

[Douglas Gilbert]

Christoph Hellwig wrote:
> On Sat, Aug 28, 2004 at 03:05:35PM +1000, Douglas Gilbert wrote:
> 
>>Here is a second cut of the patch.
>>
>>If James accepts it naturally he is free to enforce the
>>appropriate coding style.
>>
>>I believe that cleaning up scsi error processing is going
>>to be a "work in progress" for a while.
> 
> 
> Here's a version with a few small adjustments:
> 
>  - moved the code to the scsi_error.c/scsi_eh.c
>  - small style cleanups
>  - added the two helpers I suggested
>  - don't do the memset if we return failure, callers shouldn't look at
>    it then
> 
> --- linux-2.6.9-rc1-mm1/drivers/scsi/scsi_error.c	2004-08-28 12:17:33.000000000 +0200
> +++ linux-2.6.9-rc1-mm1-hch/drivers/scsi/scsi_error.c	2004-08-28 13:20:30.000000000 +0200
> @@ -1850,3 +1850,79 @@
>  	scsi_next_command(scmd);
>  	return rtn;
>  }
> +
> +/**
> + * scsi_normalize_sense - normalize main elements from either fixed or
> + *			descriptor sense data format into a common format.
> + *
> + * @sense_buffer:	byte array containing sense data returned by device
> + * @sb_len:		number of valid bytes in sense_buffer
> + * @sshdr:		pointer to instance of structure that common
> + *			elements are written to. Ignored if NULL.

Christoph,
I'm happy with this patch. Now it's my turn to be a reviewer ...
The "Ignored if NULL." sentence above is no longer correct and
should be removed. Actually it's my oversight.


So if this could be applied we could refine an error reporting
policy after the comments from Kai. Obviously if the midlevel
issues the command then it should handle all of the errors/alerts
(e.g. midlevel device discovery). At the other extreme are
pass through interfaces which want no retries and all errors
and alerts "returned to sender". Then there are the cases in
between.

Doug Gilbert

Re: [PATCH] normalize fixed and descriptor sense data

[Christoph Hellwig]

On Sun, Aug 29, 2004 at 10:36:03AM +1000, Douglas Gilbert wrote:
> I'm happy with this patch. Now it's my turn to be a reviewer ...
> The "Ignored if NULL." sentence above is no longer correct and
> should be removed. Actually it's my oversight.

Heh, indeed.  James, can you just remove " Ignored if NULL." from the
sshdr paramter description when applying?