From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47895) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8kO8-0000Qr-B8 for qemu-devel@nongnu.org; Fri, 03 Jun 2016 04:23:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8kO4-0003C7-8V for qemu-devel@nongnu.org; Fri, 03 Jun 2016 04:23:28 -0400 Received: from [59.151.112.132] (port=19671 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8kO2-0003Be-7Z for qemu-devel@nongnu.org; Fri, 03 Jun 2016 04:23:24 -0400 References: <1464062689-32156-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1464062689-32156-12-git-send-email-caoj.fnst@cn.fujitsu.com> <87inxtulso.fsf@dusky.pond.sub.org> From: Cao jin Message-ID: <57513FB2.2020708@cn.fujitsu.com> Date: Fri, 3 Jun 2016 16:28:34 +0800 MIME-Version: 1.0 In-Reply-To: <87inxtulso.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" , Jason Wang , Marcel Apfelbaum , Alex Williamson , Hannes Reinecke , Dmitry Fleytman , Paolo Bonzini , John Snow , Gerd Hoffmann Hi Markus, Thanks very much for your thorough review for the whole series, really really impressed:) On 06/01/2016 08:37 PM, Markus Armbruster wrote: > Cao jin writes: > >> msi_init() reports errors with error_report(), which is wrong >> when it's used in realize(). > > msix_init() has the same problem. Perhaps you can tackle it later. > Sure, I will take care of it after this one has passed the review. >> + error_propagate(errp, err); >> + return; >> + } else if (err && d->msi == ON_OFF_AUTO_AUTO) { >> + /* If user doesn`t set it on, switch to non-msi variant silently */ >> + error_free(err); >> + } > > The conditional is superfluous. > > We call msi_init() only if d->msi != ON_OFF_AUTO_OFF. If it sets @err > and d->msi == ON_OFF_AUTO_ON, we don't get here. Therefore, err implies > d->msi == ON_OFF_AUTO_AUTO, and the conditional can be simplified to > > } else if (err) { > error_free(err); > } > > But protecting error_free() like that is pointless, as it does nothing > when err is null. Simplify further to > > } > assert(!err || d->msi == ON_OFF_AUTO_AUTO); > /* With msi=auto, we fall back to MSI off silently */ > error_free(err); > > The assertion is more for aiding the reader than for catching mistakes. > It take me a little while to understand the following tightened error checking:) > The error checking could be tightened as follows: > > ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, > 1, true, false, &err); > assert(!ret || ret == -ENOTSUP); I guess you are assuming msi_init return 0 on success(all the following example code are), and actually it is the behaviour of msix_init, you mentioned the difference between them before. So I think it should be assert(ret < 0 || ret == -ENOTSUP); Right? And I think it is better to add a comments on it, for newbie understanding, like this: /* -EINVAL means capability overlap, which is programming error for this device, so, assert it */ Is the comment ok? And I will add a new patch in this series to make msi_init return 0 on success, and -error on failure, make it consistent with msix_init, so your example code will apply. > if (ret && d->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 || d->msi == ON_OFF_AUTO_AUTO); > /* With msi=auto, we fall back to MSI off silently */ > error_free(err); > >> + } >> + >> >> @@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) >> int sata_cap_offset; >> uint8_t *sata_cap; >> d = ICH_AHCI(dev); >> + Error *err = NULL; >> + >> + /* Although the AHCI 1.3 specification states that the first capability >> + * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9 >> + * AHCI device puts the MSI capability first, pointing to 0x80. */ >> + msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, &err); >> + if (err) { >> + /* Fall back to INTx silently */ >> + error_free(err); >> + } > > Tighter error checking: > > ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL); > /* Fall back to INTx silently on -ENOTSUP */ > assert(!ret || ret == -ENOTSUP); > > More concise, too. No need for the include "qapi/error.h" then. > Learned, and /assert/ is for -EINVAL, so I will add a comment as I mentioned above for easy understanding, So will I do for all the following example code:) > >> + if (!vmxnet3_init_msix(s)) { >> + VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); > > It doesn't replace it here, but that's appropriate, because it doesn't > touch MSI-X code, it only moves it. > will replace it when tackle msix_init >> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c >> index fa0c50c..7f9131f 100644 >> --- a/hw/pci-bridge/pci_bridge_dev.c >> +++ b/hw/pci-bridge/pci_bridge_dev.c >> @@ -54,6 +54,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >> PCIBridge *br = PCI_BRIDGE(dev); >> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); >> int err; >> + Error *local_err = NULL; >> >> pci_bridge_initfn(dev, TYPE_PCI_BUS); >> >> @@ -75,12 +76,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >> goto slotid_error; >> } >> >> - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || >> - bridge_dev->msi == ON_OFF_AUTO_ON) && >> - msi_nonbroken) { >> - err = msi_init(dev, 0, 1, true, true); >> - if (err < 0) { >> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) { > > Unnecessary churn, please use != ON_OFF_AUTO_OFF in PATCH 10. > >> + /* it means SHPC exists */ > > Does it? Why? > The comments says: /* MSI is not applicable without SHPC */. And also before the patch, we can see there are only following combination available: [shpc: on, msi:on] [shpc: on, msi:off] [shpc: off, msi: off] But there is no: [shpc: off, msi: on] So if msi != OFF, it implies shcp is on, right? > >> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c >> index c2a387a..b040575 100644 >> --- a/hw/scsi/vmw_pvscsi.c >> +++ b/hw/scsi/vmw_pvscsi.c >> @@ -1044,12 +1044,16 @@ static void >> pvscsi_init_msi(PVSCSIState *s) >> { >> int res; >> + Error *err = NULL; >> PCIDevice *d = PCI_DEVICE(s); >> >> res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS, >> - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK); >> + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err); >> if (res < 0) { >> trace_pvscsi_init_msi_fail(res); >> + error_append_hint(&err, "MSI capability fail to init," >> + " will use INTx intead\n"); >> + error_report_err(err); >> s->msi_used = false; >> } else { >> s->msi_used = true; > > This is MSI device pattern #5: on whenever possible, else off, but > report an error when falling back to off. > > Before your patch, this is pattern #2. Why do you add error reporting > here? You don't in other instances of pattern #2. > I dunno...maybe just flash into my mind randomly:-[ will get rid of it -- Yours Sincerely, Cao jin