Re: [Qemu-devel] [PATCH 2/3] scsi: replace 'tag' with 'hba_private' pointer

[Hannes Reinecke]

On 07/01/2011 10:27 AM, Paolo Bonzini wrote:
> 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?
Sure. Anything to get the patches accepted :-)

> > Signed-off-by: Hannes Reinecke<address@hidden>
> > ---
> > 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.
Ok, I'll be resending both.

Cheers,

Hannes
--
Dr. Hannes Reinecke                   zSeries & Storage
address@hidden                        +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)