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

[Hannes Reinecke <hare@suse.de>]

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<hare@suse.de>
>> ---
>> 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
hare@suse.de			      +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)