From patchwork Tue Jul 19 13:26:14 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Hannes Reinecke X-Patchwork-Id: 105476 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id E5BC8B6EE8 for ; Tue, 19 Jul 2011 23:40:11 +1000 (EST) Received: from localhost ([::1]:47263 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QjAWt-0003Le-Oj for incoming@patchwork.ozlabs.org; Tue, 19 Jul 2011 09:40:08 -0400 Received: from eggs.gnu.org ([140.186.70.92]:48380) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QjAJi-0000VB-PN for qemu-devel@nongnu.org; Tue, 19 Jul 2011 09:26:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QjAJd-0001Fp-Mz for qemu-devel@nongnu.org; Tue, 19 Jul 2011 09:26:30 -0400 Received: from cantor2.suse.de ([195.135.220.15]:43880 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QjAJc-0001FS-Cg for qemu-devel@nongnu.org; Tue, 19 Jul 2011 09:26:25 -0400 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id 53AEF8FB3C; Tue, 19 Jul 2011 15:26:22 +0200 (CEST) Message-ID: <4E2585F6.7090909@suse.de> Date: Tue, 19 Jul 2011 15:26:14 +0200 From: Hannes Reinecke User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.14) Gecko/20110221 SUSE/3.1.8 Thunderbird/3.1.8 MIME-Version: 1.0 To: Kevin Wolf References: <1311070524-13533-1-git-send-email-kwolf@redhat.com> <1311070524-13533-6-git-send-email-kwolf@redhat.com> <4E257BE6.7080108@codemonkey.ws> <4E25816E.906@redhat.com> In-Reply-To: <4E25816E.906@redhat.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4-2.6 X-Received-From: 195.135.220.15 Cc: qemu-devel@nongnu.org, David Gibson Subject: Re: [Qemu-devel] [PATCH 05/21] scsi: Add 'hba_private' to SCSIRequest X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 07/19/2011 03:06 PM, Kevin Wolf wrote: > Am 19.07.2011 14:43, schrieb Anthony Liguori: >> On 07/19/2011 05:15 AM, Kevin Wolf wrote: >>> From: Hannes Reinecke >>> >>> '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. >>> 'tag' is still being kept for tracing purposes. >>> >>> Signed-off-by: Hannes Reinecke >>> Acked-by: Paolo Bonzini >>> Signed-off-by: Kevin Wolf >>> --- >>> hw/esp.c | 2 +- >>> hw/lsi53c895a.c | 22 ++++++++-------------- >>> hw/scsi-bus.c | 9 ++++++--- >>> hw/scsi-disk.c | 4 ++-- >>> hw/scsi-generic.c | 5 +++-- >>> hw/scsi.h | 10 +++++++--- >>> hw/spapr_vscsi.c | 29 +++++++++-------------------- >>> hw/usb-msd.c | 9 +-------- >>> 8 files changed, 37 insertions(+), 53 deletions(-) >>> >>> diff --git a/hw/esp.c b/hw/esp.c >>> index aa50800..9ddd637 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, 0, lun, NULL); >>> datalen = scsi_req_enqueue(s->current_req, buf); >>> s->ti_size = datalen; >>> if (datalen != 0) { >>> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c >>> index 940b43a..69eec1d 100644 >>> --- a/hw/lsi53c895a.c >>> +++ b/hw/lsi53c895a.c >>> @@ -661,7 +661,7 @@ static lsi_request *lsi_find_by_tag(LSIState *s, uint32_t tag) >>> static void lsi_request_cancelled(SCSIRequest *req) >>> { >>> LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent); >>> - lsi_request *p; >>> + lsi_request *p = req->hba_private; >>> >>> if (s->current&& req == s->current->req) { >>> scsi_req_unref(req); >>> @@ -670,7 +670,6 @@ static void lsi_request_cancelled(SCSIRequest *req) >>> return; >>> } >>> >>> - p = lsi_find_by_tag(s, req->tag); >>> if (p) { >>> QTAILQ_REMOVE(&s->queue, p, next); >>> scsi_req_unref(req); >>> @@ -680,18 +679,12 @@ static void lsi_request_cancelled(SCSIRequest *req) >>> >>> /* Record that data is available for a queued command. Returns zero if >>> the device was reselected, nonzero if the IO is deferred. */ >>> -static int lsi_queue_tag(LSIState *s, uint32_t tag, uint32_t len) >>> +static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len) >>> { >>> - lsi_request *p; >>> - >>> - p = lsi_find_by_tag(s, tag); >>> - if (!p) { >>> - BADF("IO with unknown tag %d\n", tag); >>> - return 1; >>> - } >>> + lsi_request *p = req->hba_private; >>> >>> if (p->pending) { >>> - BADF("Multiple IO pending for tag %d\n", tag); >>> + BADF("Multiple IO pending for request %p\n", p); >>> } >>> p->pending = len; >>> /* Reselect if waiting for it, or if reselection triggers an IRQ >>> @@ -743,9 +736,9 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len) >>> LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent); >>> int out; >>> >>> - if (s->waiting == 1 || !s->current || req->tag != s->current->tag || >>> + if (s->waiting == 1 || !s->current || req->hba_private != s->current || >>> (lsi_irq_on_rsl(s)&& !(s->scntl1& LSI_SCNTL1_CON))) { >>> - if (lsi_queue_tag(s, req->tag, len)) { >>> + if (lsi_queue_req(s, req, len)) { >>> return; >>> } >>> } >>> @@ -789,7 +782,8 @@ static void lsi_do_command(LSIState *s) >>> assert(s->current == NULL); >>> s->current = qemu_mallocz(sizeof(lsi_request)); >>> s->current->tag = s->select_tag; >>> - s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun); >>> + s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, >>> + s->current); >>> >>> n = scsi_req_enqueue(s->current->req, buf); >>> if (n) { >>> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c >>> index ad6a730..8b1a412 100644 >>> --- a/hw/scsi-bus.c >>> +++ b/hw/scsi-bus.c >>> @@ -131,7 +131,8 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus) >>> return res; >>> } >>> >>> -SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun) >>> +SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, >>> + uint32_t lun, void *hba_private) >>> { >>> SCSIRequest *req; >>> >>> @@ -141,14 +142,16 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l >>> req->dev = d; >>> req->tag = tag; >>> req->lun = lun; >>> + req->hba_private = hba_private; >>> req->status = -1; >>> trace_scsi_req_alloc(req->dev->id, req->lun, req->tag); >>> return req; >>> } >>> >>> -SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun) >>> +SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun, >>> + void *hba_private) >>> { >>> - return d->info->alloc_req(d, tag, lun); >>> + return d->info->alloc_req(d, tag, lun, hba_private); >>> } >>> >>> uint8_t *scsi_req_get_buf(SCSIRequest *req) >>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c >>> index a8c7372..c2a99fe 100644 >>> --- a/hw/scsi-disk.c >>> +++ b/hw/scsi-disk.c >>> @@ -81,13 +81,13 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type); >>> static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf); >>> >>> static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, >>> - uint32_t lun) >>> + uint32_t lun, void *hba_private) >>> { >>> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d); >>> SCSIRequest *req; >>> SCSIDiskReq *r; >>> >>> - req = scsi_req_alloc(sizeof(SCSIDiskReq),&s->qdev, tag, lun); >>> + req = scsi_req_alloc(sizeof(SCSIDiskReq),&s->qdev, tag, lun, hba_private); >>> r = DO_UPCAST(SCSIDiskReq, req, req); >>> r->iov.iov_base = qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE); >>> return req; >>> diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c >>> index 8e59c7e..90345a7 100644 >>> --- a/hw/scsi-generic.c >>> +++ b/hw/scsi-generic.c >>> @@ -96,11 +96,12 @@ static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len) >>> return size; >>> } >>> >>> -static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun) >>> +static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun, >>> + void *hba_private) >>> { >>> SCSIRequest *req; >>> >>> - req = scsi_req_alloc(sizeof(SCSIGenericReq), d, tag, lun); >>> + req = scsi_req_alloc(sizeof(SCSIGenericReq), d, tag, lun, hba_private); >>> return req; >>> } >>> >>> diff --git a/hw/scsi.h b/hw/scsi.h >>> index c1dca35..6b15bbc 100644 >>> --- a/hw/scsi.h >>> +++ b/hw/scsi.h >>> @@ -43,6 +43,7 @@ struct SCSIRequest { >>> } cmd; >>> BlockDriverAIOCB *aiocb; >>> bool enqueued; >>> + void *hba_private; >>> QTAILQ_ENTRY(SCSIRequest) next; >>> }; >>> >>> @@ -67,7 +68,8 @@ struct SCSIDeviceInfo { >>> DeviceInfo qdev; >>> scsi_qdev_initfn init; >>> void (*destroy)(SCSIDevice *s); >>> - SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun); >>> + SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun, >>> + void *hba_private); >>> void (*free_req)(SCSIRequest *req); >>> int32_t (*send_command)(SCSIRequest *req, uint8_t *buf); >>> void (*read_data)(SCSIRequest *req); >>> @@ -138,8 +140,10 @@ extern const struct SCSISense sense_code_LUN_FAILURE; >>> int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed); >>> int scsi_sense_valid(SCSISense sense); >>> >>> -SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun); >>> -SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun); >>> +SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, >>> + uint32_t lun, void *hba_private); >>> +SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun, >>> + void *hba_private); >>> int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf); >>> void scsi_req_free(SCSIRequest *req); >>> SCSIRequest *scsi_req_ref(SCSIRequest *req); >>> diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c >>> index 1c901ef..9e1cb2e 100644 >>> --- a/hw/spapr_vscsi.c >>> +++ b/hw/spapr_vscsi.c >>> @@ -121,7 +121,7 @@ static struct vscsi_req *vscsi_get_req(VSCSIState *s) >>> return NULL; >>> } >>> >>> -static void vscsi_put_req(VSCSIState *s, vscsi_req *req) >>> +static void vscsi_put_req(vscsi_req *req) >>> { >>> if (req->sreq != NULL) { >>> scsi_req_unref(req->sreq); >> >> This breaks the build: >> >> make[1]: Nothing to be done for `all'. >> CC ppc64-softmmu/spapr_vscsi.o >> /home/anthony/git/qemu/hw/spapr_vscsi.c: In function >> ‘vscsi_command_complete’: >> /home/anthony/git/qemu/hw/spapr_vscsi.c:535:34: error: ‘s’ undeclared >> (first use in this function) >> /home/anthony/git/qemu/hw/spapr_vscsi.c:535:34: note: each undeclared >> identifier is reported only once for each function it appears in > > Adding Hannes to CC. > >> This file is only built when libfdt is installed which is probably why >> you didn't catch it. > > Yes, I did build all targets, but probably don't have most optional > dependencies installed. I guess I should install some more libraries on > that machine. > > But as you said, there is no libfdt package in RHEL, so it will not be > among them, and I'd like to avoid installing things from source. > Fix is attached. How to proceed? Do you require a new patch or can you fix it up yourself? Cheers, Hannes diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c index 9e1cb2e..646b1e3 100644 --- a/hw/spapr_vscsi.c +++ b/hw/spapr_vscsi.c @@ -521,6 +521,7 @@ static void vscsi_transfer_data(SCSIRequest *sreq, uint32_t len) /* Callback to indicate that the SCSI layer has completed a transfer. */ static void vscsi_command_complete(SCSIRequest *sreq, uint32_t status) { + VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent); vscsi_req *req = sreq->hba_private; int32_t res_in = 0, res_out = 0;