scsi-disk: correct SCSI error reporting

PATCH Applied Laurent Vivier 2 participants 3 messages Oct 9, 2008 – Oct 11, 2008

lore

Share & tools

lore

Laurent Vivier

author

2008-10-09 16:35 UTC #0 lore raw

PATCH v2 1/1 scsi-disk: correct SCSI error reporting

Hi,

scsi-disk.c seems to not report to SCSI controller (through completion() routine) the good error value.

I've tested this by using a little program trying to read beyond the end of the disk (see attachment read10.c)

On real disk:

# ./read10 /dev/sg0
READ_10 duration=40 millisecs, resid=0, msg_status=0 status=2 sense=0
driver_status 8

If I use this command with qemu and a scsi disk, the result is:

# ./read10 /dev/sg0
sym0: bad DSA (5da40ff) in done queue.
sd 0:0:0:0: ABORT operation started
sym0: SCSI BUS reset detected.
sym0: SCSI BUS has been reset.
sd 0:0:0:0: ABORT operation complete.
sd 0:0:0:0: ABORT operation started
sd 0:0:0:0: ABORT operation failed.
sd 0:0:0:0: DEVICE RESET operation started
 target0:0:0: control msgout: c.
lsi_scsi: error: Unimplemented message 0x0c
 target0:0:0: has been reset
sd 0:0:0:0: DEVICE RESET operation complete.
sd 0:0:0:0: DEVICE RESET operation started
 target0:0:0: control msgout: c.
lsi_scsi: error: Unimplemented message 0x0c
 target0:0:0: has been reset
sd 0:0:0:0: DEVICE RESET operation complete.
sd 0:0:0:0: M_REJECT received (0:0).
sd 0:0:0:0: M_REJECT received (0:0).
READ_10 duration=30032 millisecs, resid=0, msg_status=0 status=0 sense=0
driver_status 0

The following patch corrects this issue by sending to the controller not the sense key but the status code.

The result of the command is now:

# ./read10 /dev/sg0
READ_10 duration=0 millisecs, resid=0, msg_status=0 status=2 sense=0
driver_status 8

This patch has been tested with linux (x86_64 and sparc) and windows XP.

The VERIFY commands has been implemented (empty) because previously it was simply ignored (and it is needed by windows to format disks).

Changelog:

[v2] The v1 was very close to the (reversed) patch of Marcelo Tosatti (r4260) which was breaking sparc scsi support. In fact, management of invalid LUN was wrong: on an invalid LUN, disk sends a SCSI status of "CHECK_CONDITION" and the driver sends "REQUEST_SENSE" to retrieve the sense key. The disk must accept and answer to this request (to send the sense key ILLEGAL_REQUEST). Moreover, INQUIRY with an invalid LUN must answer with PERIPHERAL QUALIFIER field set to 011b and PERIPHERAL DEVICE TYPE field set to 1Fh.

Laurent

Signature · 3 lines

----------------- [email protected]  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <scsi/sg.h>

/* modified file from /usr/share/doc/sg3-utils/examples/sg_simple16.c
* * Copyright (C) 2001 D. Gilbert
* */

#define READ10_REPLY_LEN 512
#define READ10_CMD_LEN 10

#define EBUFF_SZ 256

int main(int argc, char * argv[])

{
    int sg_fd, k, ok;
    unsigned char r10CmdBlk [READ10_CMD_LEN] =
                {0x28, 0, 0x7f, 0xff, 0xff, 0xff, 0, 0, 1, 0};
    sg_io_hdr_t io_hdr;
    char * file_name = 0;
    char ebuff[EBUFF_SZ];
    unsigned char inBuff[READ10_REPLY_LEN];
    unsigned char sense_buffer[32];

    for (k = 1; k < argc; ++k) {
        if (*argv[k] == '-') {
            printf("Unrecognized switch: %s\n", argv[k]);
            file_name = 0;
            break;
        }
        else if (0 == file_name)
            file_name = argv[k];
        else {
            printf("too many arguments\n");
            file_name = 0;
            break;
        }
    }
    if (0 == file_name) {
        printf("Usage: 'sg_simple10 <sg_device>'\n");
        return 1;
    }

    if ((sg_fd = open(file_name, O_RDWR)) < 0) {
        snprintf(ebuff, EBUFF_SZ,
                 "sg_simple10: error opening file: %s", file_name);
        perror(ebuff);
        return 1;
    }
    /* Prepare READ_10 command */
    memset(&io_hdr, 0, sizeof(sg_io_hdr_t));
    io_hdr.interface_id = 'S';
    io_hdr.cmd_len = sizeof(r10CmdBlk);
    /* io_hdr.iovec_count = 0; */  /* memset takes care of this */
    io_hdr.mx_sb_len = sizeof(sense_buffer);
    io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
    io_hdr.dxfer_len = READ10_REPLY_LEN;
    io_hdr.dxferp = inBuff;
    io_hdr.cmdp = r10CmdBlk;
    io_hdr.sbp = sense_buffer;
    io_hdr.timeout = 20000;     /* 20000 millisecs == 20 seconds */
    /* io_hdr.flags = 0; */     /* take defaults: indirect IO, etc */
    /* io_hdr.pack_id = 0; */
    /* io_hdr.usr_ptr = NULL; */

    if (ioctl(sg_fd, SG_IO, &io_hdr) < 0) {
        perror("sg_simple10: Inquiry SG_IO ioctl error");
        close(sg_fd);
        return 1;
    }

    printf("READ_10 duration=%u millisecs, resid=%d, msg_status=%d status=%d sense=%d driver_status %x\n",
           io_hdr.duration, io_hdr.resid, (int)io_hdr.msg_status,
           io_hdr.status, sense_buffer[2], io_hdr.driver_status);

    close(sg_fd);
    return 0;
}

Signed-off-by Laurent Vivier <[email protected]>

---
 hw/scsi-disk.c |   42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

hw/scsi-disk.c +27 ~~-15~~

Index: qemu/hw/scsi-disk.c
===================================================================
@@ -34,6 +34,9 @@ do { fprintf(stderr, "scsi-disk: " fmt ,
 #define SENSE_HARDWARE_ERROR  4
 #define SENSE_ILLEGAL_REQUEST 5
 
+#define STATUS_GOOD            0
+#define STATUS_CHECK_CONDITION 2
+
 #define SCSI_DMA_BUF_SIZE    131072
 
 typedef struct SCSIRequest {

@@ -124,15 +127,15 @@ static SCSIRequest *scsi_find_request(SC
 }
 
 /* Helper function for command completion.  */
-static void scsi_command_complete(SCSIRequest *r, int sense)
+static void scsi_command_complete(SCSIRequest *r, int status, int sense)
 {
     SCSIDeviceState *s = r->dev;
     uint32_t tag;
-    DPRINTF("Command complete tag=0x%x sense=%d\n", r->tag, sense);
+    DPRINTF("Command complete tag=0x%x status=%d sense=%d\n", r->tag, status, sense);
     s->sense = sense;
     tag = r->tag;
     scsi_remove_request(r);
-    s->completion(s->opaque, SCSI_REASON_DONE, tag, sense);
+    s->completion(s->opaque, SCSI_REASON_DONE, tag, status);
 }
 
 /* Cancel a pending data transfer.  */

@@ -157,7 +160,8 @@ static void scsi_read_complete(void * op
 
     if (ret) {
         DPRINTF("IO error\n");
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        s->completion(s->opaque, SCSI_REASON_DATA, r->tag, 0);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_NO_SENSE);
         return;
     }
     DPRINTF("Data ready tag=0x%x len=%d\n", r->tag, r->buf_len);

@@ -176,7 +180,7 @@ static void scsi_read_data(SCSIDevice *d
     if (!r) {
         BADF("Bad read tag 0x%x\n", tag);
         /* ??? This is the wrong error.  */
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
         return;
     }
     if (r->sector_count == (uint32_t)-1) {

@@ -187,7 +191,7 @@ static void scsi_read_data(SCSIDevice *d
     }
     DPRINTF("Read sector_count=%d\n", r->sector_count);
     if (r->sector_count == 0) {
-        scsi_command_complete(r, SENSE_NO_SENSE);
+        scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
         return;
     }

@@ -199,7 +203,7 @@ static void scsi_read_data(SCSIDevice *d
     r->aiocb = bdrv_aio_read(s->bdrv, r->sector, r->dma_buf, n,
                              scsi_read_complete, r);
     if (r->aiocb == NULL)
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
     r->sector += n;
     r->sector_count -= n;
 }

@@ -217,7 +221,7 @@ static void scsi_write_complete(void * o
 
     r->aiocb = NULL;
     if (r->sector_count == 0) {
-        scsi_command_complete(r, SENSE_NO_SENSE);
+        scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
     } else {
         len = r->sector_count * 512;
         if (len > SCSI_DMA_BUF_SIZE) {

@@ -241,7 +245,7 @@ static int scsi_write_data(SCSIDevice *d
     r = scsi_find_request(s, tag);
     if (!r) {
         BADF("Bad write tag 0x%x\n", tag);
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
         return 1;
     }
     if (r->aiocb)

@@ -251,7 +255,8 @@ static int scsi_write_data(SCSIDevice *d
         r->aiocb = bdrv_aio_write(s->bdrv, r->sector, r->dma_buf, n,
                                   scsi_write_complete, r);
         if (r->aiocb == NULL)
-            scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+            scsi_command_complete(r, STATUS_CHECK_CONDITION,
+                                  SENSE_HARDWARE_ERROR);
         r->sector += n;
         r->sector_count -= n;
     } else {

@@ -344,7 +349,8 @@ static int32_t scsi_send_command(SCSIDev
     if (lun || buf[1] >> 5) {
         /* Only LUN 0 supported.  */
         DPRINTF("Unimplemented LUN %d\n", lun ? lun : buf[1] >> 5);
-        goto fail;
+        if (command != 0x03 && command != 0x12) /* REQUEST SENSE and INQUIRY */
+            goto fail;
     }
     switch (command) {
     case 0x0:

@@ -487,7 +493,10 @@ static int32_t scsi_send_command(SCSIDev
             }
         }
 	memset(outbuf, 0, 36);
-	if (bdrv_get_type_hint(s->bdrv) == BDRV_TYPE_CDROM) {
+
+        if (lun || buf[1] >> 5) {
+            outbuf[0] = 0x7f;	/* LUN not supported */
+	} else if (bdrv_get_type_hint(s->bdrv) == BDRV_TYPE_CDROM) {
 	    outbuf[0] = 5;
             outbuf[1] = 0x80;
 	    memcpy(&outbuf[16], "QEMU CD-ROM    ", 16);

@@ -670,7 +679,7 @@ static int32_t scsi_send_command(SCSIDev
             outbuf[7] = 0;
             r->buf_len = 8;
         } else {
-            scsi_command_complete(r, SENSE_NOT_READY);
+            scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_NOT_READY);
             return 0;
         }
 	break;

@@ -754,14 +763,17 @@ static int32_t scsi_send_command(SCSIDev
         outbuf[3] = 8;
         r->buf_len = 16;
         break;
+    case 0x2f:
+        DPRINTF("Verify (sector %d, count %d)\n", lba, len);
+        break;
     default:
 	DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
     fail:
-        scsi_command_complete(r, SENSE_ILLEGAL_REQUEST);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_ILLEGAL_REQUEST);
 	return 0;
     }
     if (r->sector_count == 0 && r->buf_len == 0) {
-        scsi_command_complete(r, SENSE_NO_SENSE);
+        scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
     }
     len = r->sector_count * 512 + r->buf_len;
     if (is_write) {

Blue Swirl

2008-10-09 18:12 UTC #1 lore raw

Quoted 55 lines · Laurent Vivier

Hi,
scsi-disk.c seems to not report to SCSI controller (through completion() routine) the good error value.
I've tested this by using a little program trying to read beyond the end of the disk (see attachment read10.c)
On real disk:
 # ./read10 /dev/sg0
 READ_10 duration=40 millisecs, resid=0, msg_status=0 status=2 sense=0
 driver_status 8
If I use this command with qemu and a scsi disk, the result is:
 # ./read10 /dev/sg0
 sym0: bad DSA (5da40ff) in done queue.
 sd 0:0:0:0: ABORT operation started
 sym0: SCSI BUS reset detected.
 sym0: SCSI BUS has been reset.
 sd 0:0:0:0: ABORT operation complete.
 sd 0:0:0:0: ABORT operation started
 sd 0:0:0:0: ABORT operation failed.
 sd 0:0:0:0: DEVICE RESET operation started
  target0:0:0: control msgout: c.
 lsi_scsi: error: Unimplemented message 0x0c
  target0:0:0: has been reset
 sd 0:0:0:0: DEVICE RESET operation complete.
 sd 0:0:0:0: DEVICE RESET operation started
  target0:0:0: control msgout: c.
 lsi_scsi: error: Unimplemented message 0x0c
  target0:0:0: has been reset
 sd 0:0:0:0: DEVICE RESET operation complete.
 sd 0:0:0:0: M_REJECT received (0:0).
 sd 0:0:0:0: M_REJECT received (0:0).
 READ_10 duration=30032 millisecs, resid=0, msg_status=0 status=0 sense=0
 driver_status 0
The following patch corrects this issue by sending to the controller not the sense key but the status code.
The result of the command is now:
 # ./read10 /dev/sg0
 READ_10 duration=0 millisecs, resid=0, msg_status=0 status=2 sense=0
 driver_status 8
This patch has been tested with linux (x86_64 and sparc) and windows XP.
The VERIFY commands has been implemented (empty) because previously it was simply ignored (and it is needed by windows to format disks).
Changelog:

Quoted 8 lines

[v2] The v1 was very close to the (reversed) patch of Marcelo Tosatti (r4260) which was breaking sparc scsi support. In fact, management of invalid LUN was wrong: on an invalid LUN, disk sends a SCSI status of "CHECK_CONDITION" and the driver sends "REQUEST_SENSE" to retrieve the sense key. The disk must accept and answer to this request (to send the sense key ILLEGAL_REQUEST). Moreover, INQUIRY with an invalid LUN must answer with PERIPHERAL QUALIFIER field set to 011b and PERIPHERAL DEVICE TYPE field set to 1Fh.

This patch fixes LUN handling and does not cause problems like the earlier one. I'd like to apply this in a few days, unless there are objections.

Blue Swirl

Applied

2008-10-11 09:34 UTC #2 lore raw

In reply to Laurent Vivier

Quoted 4 lines · Laurent Vivier

Hi,
scsi-disk.c seems to not report to SCSI controller (through completion() routine) the good error value.

Thanks, applied as r5455.