From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:54352)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1QHEUW-00029A-Ih
	for qemu-devel@nongnu.org; Tue, 03 May 2011 08:14:13 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1QHEUV-0000nv-MX
	for qemu-devel@nongnu.org; Tue, 03 May 2011 08:14:12 -0400
Received: from mail-gx0-f173.google.com ([209.85.161.173]:48398)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1QHEUV-0000nr-Hf
	for qemu-devel@nongnu.org; Tue, 03 May 2011 08:14:11 -0400
Received: by gxk26 with SMTP id 26so769gxk.4
	for <qemu-devel@nongnu.org>; Tue, 03 May 2011 05:14:11 -0700 (PDT)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <4DBFF18E.3000602@redhat.com>
Date: Tue, 03 May 2011 14:14:06 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1301980051-20866-1-git-send-email-david@gibson.dropbear.id.au>
	<4D9B2060.3060000@redhat.com>
In-Reply-To: <4D9B2060.3060000@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] scsi-generic: Remove bogus double complete
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-devel@nongnu.org, David Gibson <david@gibson.dropbear.id.au>

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