[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and f
|
From: |
Cao jin |
|
Subject: |
Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it |
|
Date: |
Fri, 3 Jun 2016 16:28:34 +0800 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
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 <address@hidden> 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