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