normalize fixed and descriptor sense data

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

maybe scsi_sense_desc insted?

include/scsi/scsi.h269rc1sdesc renamed from include/scsi/scsi.h +8 -0

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

include/scsi/scsi.h269rc1sdesc renamed from include/scsi/scsi.h +5 -0

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

drivers/scsi/scsi_lib.c269rc1sdesc renamed from drivers/scsi/scsi_lib.c +21 -0

+/**
+ *     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.

drivers/scsi/scsi_lib.c269rc1sdesc renamed from drivers/scsi/scsi_lib.c +5 -0

+       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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Christoph Hellwig

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

Quoted 3 lines

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.

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.

Quoted 7 lines · Douglas Gilbert

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

drivers/scsi/scsi_error.c +0 -0

@@ -1850,3 +1850,79 @@
 	scsi_next_command(scmd);
 	return rtn;

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.