From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44662) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QcZ4L-0000AE-0f for qemu-devel@nongnu.org; Fri, 01 Jul 2011 04:27:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QcZ4I-0003kf-MT for qemu-devel@nongnu.org; Fri, 01 Jul 2011 04:27:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41386) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QcZ4I-0003k2-4C for qemu-devel@nongnu.org; Fri, 01 Jul 2011 04:27:18 -0400 Message-ID: <4E0D84DF.7030303@redhat.com> Date: Fri, 01 Jul 2011 10:27:11 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1309506172-17762-1-git-send-email-hare@suse.de> <1309506172-17762-2-git-send-email-hare@suse.de> <1309506172-17762-3-git-send-email-hare@suse.de> In-Reply-To: <1309506172-17762-3-git-send-email-hare@suse.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] scsi: replace 'tag' with 'hba_private' pointer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hannes Reinecke Cc: Alexander Graf , qemu-devel@nongnu.org, kvm@vger.kernel.org, Stefan Haynoczi On 07/01/2011 09:42 AM, Hannes Reinecke wrote: > 'tag' is just an abstraction to identify the command > from the driver. So we should make that explicit by > replacing 'tag' with a driver-defined pointer 'hba_private'. > This saves the lookup for driver handling several commands > in parallel. This makes tracing a bit harder to follow. Perhaps you can keep the transport tag (a uint64_t) in the SCSIRequest for debugging purposes? > Signed-off-by: Hannes Reinecke > --- > hw/esp.c | 2 +- > hw/lsi53c895a.c | 17 ++++++++--------- > hw/scsi-bus.c | 22 +++++++++++----------- > hw/scsi-disk.c | 5 ++--- > hw/scsi-generic.c | 4 ++-- > hw/scsi.h | 8 ++++---- > hw/spapr_vscsi.c | 41 ++++++++++++----------------------------- > hw/usb-msd.c | 10 +++++----- > trace-events | 14 +++++++------- > 9 files changed, 52 insertions(+), 71 deletions(-) > > diff --git a/hw/esp.c b/hw/esp.c > index 6d3f5d2..912ff89 100644 > --- a/hw/esp.c > +++ b/hw/esp.c > @@ -244,7 +244,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid) > > DPRINTF("do_busid_cmd: busid 0x%x\n", busid); > lun = busid& 7; > - s->current_req = scsi_req_new(s->current_dev, 0, lun); > + s->current_req = scsi_req_new(s->current_dev, lun, s); Might as well pass NULL here. The hba_private value is basically unnecessary when the adapter doesn't support tagged command queuing. > diff --git a/hw/usb-msd.c b/hw/usb-msd.c > index 86582cc..4e2ea03 100644 > --- a/hw/usb-msd.c > +++ b/hw/usb-msd.c > @@ -216,8 +216,8 @@ static void usb_msd_transfer_data(SCSIRequest *req, uint32_t len) > MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent); > USBPacket *p = s->packet; > > - if (req->tag != s->tag) { > - fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag); > + if (req->hba_private != s) { > + fprintf(stderr, "usb-msd: Unexpected SCSI command 0x%p\n", req); > } Same here, just pass NULL and remove these ifs. Otherwise looks like a very good idea. Paolo