[Qemu-devel] [PATCH] scsi-generic: don't report negative length to the SCSI adapter

[Qemu-devel] [PATCH] scsi-generic: don't report negative length to the SCSI adapter

[Bernhard Kohl]

Some drivers report an incorrect number for 'resid'. I found that for
MODE_SENSE(6) on an IET iSCSI device. This device reports the
available mode data length minus actually transferred length.

This is already a known problem:
http://tldp.org/HOWTO/SCSI-Generic-HOWTO/x356.html

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-generic.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 9538027..836678c 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -169,7 +169,11 @@ static void scsi_read_complete(void * opaque, int ret)
         return;
     }
     len = r->io_header.dxfer_len - r->io_header.resid;
-    DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
+    if (len < 0) {
+        len = r->io_header.dxfer_len;
+    }
+    DPRINTF("Data ready tag=0x%x len=%d dxfer_len=%d resid=%d\n",
+            r->req.tag, len, r->io_header.dxfer_len, r->io_header.resid);
 
     r->len = -1;
     r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
-- 
1.7.2.2

Re: [Qemu-devel] [PATCH] scsi-generic: don't report negative length to the SCSI adapter

[Kevin Wolf]

Am 02.09.2010 15:27, schrieb Bernhard Kohl:
> Some drivers report an incorrect number for 'resid'. I found that for
> MODE_SENSE(6) on an IET iSCSI device. This device reports the
> available mode data length minus actually transferred length.
> 
> This is already a known problem:
> http://tldp.org/HOWTO/SCSI-Generic-HOWTO/x356.html
> 
> Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
> ---
>  hw/scsi-generic.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
> index 9538027..836678c 100644
> --- a/hw/scsi-generic.c
> +++ b/hw/scsi-generic.c
> @@ -169,7 +169,11 @@ static void scsi_read_complete(void * opaque, int ret)
>          return;
>      }
>      len = r->io_header.dxfer_len - r->io_header.resid;
> -    DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
> +    if (len < 0) {
> +        len = r->io_header.dxfer_len;
> +    }
> +    DPRINTF("Data ready tag=0x%x len=%d dxfer_len=%d resid=%d\n",
> +            r->req.tag, len, r->io_header.dxfer_len, r->io_header.resid);
>  
>      r->len = -1;
>      r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);

Can we add a comment why len can become < 0 and that this is a
workaround for buggy drivers? It's in your commit message, but I prefer
this kind of things to be explained in the code.

Also, are we sure that broken drivers always return negative numbers
when their result is wrong, or can we still get incorrect results?

Kevin

Re: [Qemu-devel] [PATCH] scsi-generic: don't report negative length to the SCSI adapter

[Bernhard Kohl]

Am 02.09.2010 16:32, schrieb ext Kevin Wolf:
> Can we add a comment why len can become<  0 and that this is a
> workaround for buggy drivers? It's in your commit message, but I prefer
> this kind of things to be explained in the code.
>    

Yes I will do that, if we decide to apply this patch (see below).

> Also, are we sure that broken drivers always return negative numbers
> when their result is wrong, or can we still get incorrect results?

Obviously there are currently no additional problems with wrong 'resid'
values. I found the problem with an older IET version (1.4.18)
which was the last one available from rpmfusion (kmod-iscsitarget,
iscsitarget, built for kernel 2.6.31.12). After that I switched
to STGT which is part of Fedora (scsi-target-utils). I don't know
how the current IET (1.4.20.2) behaves.

If the 'resid' is not reported correctly, there is no way to calculate
the correct received data length without analyzing the content of the
SCSI data. Negative length only occurred if the allocation length in
the MODE SENSE command was to small. But negative length confuses the
adapter driver.

I propose to skip this patch as long as there are no news about IET.
Sorry for the confusion. The patch was still in my queue since March.

Bernhard