From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34178) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjj4J-0002oJ-NU for qemu-devel@nongnu.org; Tue, 13 Sep 2016 04:27:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjj4E-00037H-39 for qemu-devel@nongnu.org; Tue, 13 Sep 2016 04:27:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52756) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjj4D-00036v-Rj for qemu-devel@nongnu.org; Tue, 13 Sep 2016 04:27:46 -0400 From: Markus Armbruster References: <1471944454-13895-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1471944454-13895-4-git-send-email-caoj.fnst@cn.fujitsu.com> <87oa3tw7oz.fsf@dusky.pond.sub.org> <57D79700.3060605@cn.fujitsu.com> Date: Tue, 13 Sep 2016 10:27:41 +0200 In-Reply-To: <57D79700.3060605@cn.fujitsu.com> (Cao jin's message of "Tue, 13 Sep 2016 14:04:48 +0800") Message-ID: <87zincdx1e.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 3/5] pci: Convert msix_init() to Error and fix callers to check it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin Cc: Jiri Pirko , "Michael S. Tsirkin" , Jason Wang , qemu-devel@nongnu.org, Dmitry Fleytman , Alex Williamson , Hannes Reinecke , Marcel Apfelbaum , Paolo Bonzini , Gerd Hoffmann Cao jin writes: > On 09/12/2016 09:47 PM, Markus Armbruster wrote: >> Cao jin writes: [...] >>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c >>> index 188f954..4280c5d 100644 >>> --- a/hw/usb/hcd-xhci.c >>> +++ b/hw/usb/hcd-xhci.c >>> @@ -3594,25 +3594,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) >>> dev->config[PCI_CACHE_LINE_SIZE] = 0x10; >>> dev->config[0x60] = 0x30; /* release number */ >>> >>> - usb_xhci_init(xhci); >>> - >>> - if (xhci->msi != ON_OFF_AUTO_OFF) { >>> - ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); >>> - /* Any error other than -ENOTSUP(board's MSI support is broken) >>> - * is a programming error */ >>> - assert(!ret || ret == -ENOTSUP); >>> - if (ret && xhci->msi == ON_OFF_AUTO_ON) { >>> - /* Can't satisfy user's explicit msi=on request, fail */ >>> - error_append_hint(&err, "You have to use msi=auto (default) or " >>> - "msi=off with this machine type.\n"); >>> - error_propagate(errp, err); >>> - return; >>> - } >>> - assert(!err || xhci->msi == ON_OFF_AUTO_AUTO); >>> - /* With msi=auto, we fall back to MSI off silently */ >>> - error_free(err); >>> - } >>> - >>> if (xhci->numintrs > MAXINTRS) { >>> xhci->numintrs = MAXINTRS; >>> } >>> @@ -3622,21 +3603,60 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) >>> if (xhci->numintrs < 1) { >>> xhci->numintrs = 1; >>> } >>> + >>> if (xhci->numslots > MAXSLOTS) { >>> xhci->numslots = MAXSLOTS; >>> } >>> if (xhci->numslots < 1) { >>> xhci->numslots = 1; >>> } >>> + >>> if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) { >>> xhci->max_pstreams_mask = 7; /* == 256 primary streams */ >>> } else { >>> xhci->max_pstreams_mask = 0; >>> } >>> >>> - xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci); >>> + if (xhci->msi != ON_OFF_AUTO_OFF) { >>> + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); >>> + /* Any error other than -ENOTSUP(board's MSI support is broken) >>> + * is a programming error */ >>> + assert(!ret || ret == -ENOTSUP); >>> + if (ret && xhci->msi == ON_OFF_AUTO_ON) { >>> + /* Can't satisfy user's explicit msi=on request, fail */ >>> + error_append_hint(&err, "You have to use msi=auto (default) or " >>> + "msi=off with this machine type.\n"); >>> + error_propagate(errp, err); >>> + return; >>> + } >>> + assert(!err || xhci->msi == ON_OFF_AUTO_AUTO); >>> + /* With msi=auto, we fall back to MSI off silently */ >>> + error_free(err); >>> + } >> >> Can you explain why you're moving this code? >> > > Sorry I forget to mention this: msi_init() uses xhci->numintrs, but > there is value checking/correcting on xhci->numintrs, it should be > done before using. If you do the move in a separate patch before this one, you can explain it in its commit message. Easier to review that way.