[Qemu-devel] [PATCH] scsi-generic: Remove bogus double complete

[Qemu-devel] [PATCH] scsi-generic: Remove bogus double complete

[David Gibson]

From: Ben Herrenschmidt <benh@kernel.crashing.org>

scsi-generic scsi_read_complete() should not -both- call the client
complete callback with SCSI_REASON_DATA -and- call
scsi_command_complete().  The former will cause the client to queue a
new read or write request, while the later will free the request data
structure, thus causing the new read or write request to use a
freed/stale structure when it completes.

This patch fixes the bug, fixing a crash with scsi-generic & RHEL5.5
installer.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/scsi-generic.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 9be1cca..c06f5df 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -173,8 +173,6 @@ static void scsi_read_complete(void * opaque, int ret)
 
     r->len = -1;
     r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
-    if (len == 0)
-        scsi_command_complete(r, 0);
 }
 
 /* Read more data from scsi device into buffer.  */
-- 
1.7.1

Re: [Qemu-devel] [PATCH] scsi-generic: Remove bogus double complete

[Kevin Wolf]

Am 05.04.2011 07:07, schrieb David Gibson:
> From: Ben Herrenschmidt <benh@kernel.crashing.org>
> 
> scsi-generic scsi_read_complete() should not -both- call the client
> complete callback with SCSI_REASON_DATA -and- call
> scsi_command_complete().  The former will cause the client to queue a
> new read or write request, while the later will free the request data
> structure, thus causing the new read or write request to use a
> freed/stale structure when it completes.
> 
> This patch fixes the bug, fixing a crash with scsi-generic & RHEL5.5
> installer.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/scsi-generic.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
> index 9be1cca..c06f5df 100644
> --- a/hw/scsi-generic.c
> +++ b/hw/scsi-generic.c
> @@ -173,8 +173,6 @@ static void scsi_read_complete(void * opaque, int ret)
>  
>      r->len = -1;
>      r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
> -    if (len == 0)
> -        scsi_command_complete(r, 0);
>  }
>  
>  /* Read more data from scsi device into buffer.  */

I have a hard time each time I try to understand this SCSI stuff without
reading a lot of code and specs. What I would have expected is this:

if (len == 0) {
    scsi_command_complete(r, 0);
} else {
    r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
}

This would fix the problem that both functions are called, but still use
SCSI_REASON_DONE if no data is transferred. This is similar to what
scsi-disk seems to be doing. However, if you can explain to me why your
version is more correct, I'll gladly take it.

Kevin

Re: [Qemu-devel] [PATCH] scsi-generic: Remove bogus double complete

[Paolo Bonzini]

On 04/05/2011 04:00 PM, Kevin Wolf wrote:
> I have a hard time each time I try to understand this SCSI stuff without
> reading a lot of code and specs. What I would have expected is this:
>
> if (len == 0) {
>      scsi_command_complete(r, 0);
> } else {
>      r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
> }
>
> This would fix the problem that both functions are called, but still use
> SCSI_REASON_DONE if no data is transferred. This is similar to what
> scsi-disk seems to be doing. However, if you can explain to me why your
> version is more correct, I'll gladly take it.

I agree with Kevin that his version seems more correct.  The purpose of 
the code is to trigger a phase mismatch for the lsi controller, and that 
would probably not happen with your version.  s->current->dma_len would 
never go down to 0 if it were really a short transfer:

     s->current->dma_len -= count;
     if (s->current->dma_len == 0) {
         s->current->dma_buf = NULL;
         if (out) {
             /* Write the data.  */
             dev->info->write_data(dev, s->current->tag);
         } else {
             /* Request any remaining data.  */
             dev->info->read_data(dev, s->current->tag);
         }
     } else {
         s->current->dma_buf += count;
         lsi_resume_script(s);
     }

so the SCRIPTS would deadlock, waiting for the full transfer to happen, 
and the SCSI_REASON_DONE callback would never be sent.

Even with spapr_vscsi, however, what would happen is that the 
SCSI_REASON_DATA does nothing except calling sdev->info->read_data (i.e. 
scsi_read_data) again.  This would then call scsi_command_complete and 
send the SCSI_REASON_DONE callback.  So for spapr_vscsi there should be 
no difference between the two cases.

I'll include Kevin's patch into my upcoming SCSI series.

Paolo