* [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup
@ 2017-06-06 11:26 Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 1/7] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel
v3:
* patch2: explain the specified means of the return value, also
improve the commit message. [Marcel Apfelbaum]
* patch3: simplify the subject and commit message, fix another
wrong assert. [Marcel Apfelbaum]
* patch4: adjust the subject.
* patch5: fix a wrong optimization for errp. [Eduardo Habkost]
* patch7: a new patch that converts shpc_init() to Error in order
to propagate the error better.
v2:
* patch1: subject and commit message was rewrited by markus.
* patch2: comment was added to pci_add_capability2().
* patch3: a new patch that fix the wrong return value judgment condition.
* patch4: a new patch that fix code style problems.
* patch5: add an errp argument for pci_add_capability to pass
error for its callers.
* patch6: convert part of pci-bridge device to realize.
v1:
* patch1: fix unreasonable return value check
Mao Zhongyi (7):
pci: Clean up error checking in pci_add_capability()
pci: Add comment for pci_add_capability2()
pci: Fix the return value checking
net/eepro100: Fix code style
pci: Make errp the last parameter of pci_add_capability()
pci: Convert to realize
pci: Convert shpc_init() to Error
hw/i386/amd_iommu.c | 24 ++++++++----
hw/net/e1000e.c | 9 ++++-
hw/net/eepro100.c | 80 +++++++++++++++++++++-----------------
hw/pci-bridge/i82801b11.c | 12 +++---
hw/pci-bridge/pci_bridge_dev.c | 21 ++++------
hw/pci-bridge/pcie_root_port.c | 15 +++----
hw/pci-bridge/xio3130_downstream.c | 20 +++++-----
hw/pci-bridge/xio3130_upstream.c | 20 +++++-----
hw/pci/pci.c | 20 +++++-----
hw/pci/pci_bridge.c | 8 +++-
hw/pci/pcie.c | 15 +++++--
hw/pci/shpc.c | 10 +++--
hw/pci/slotid_cap.c | 12 ++++--
hw/usb/hcd-xhci.c | 2 +-
hw/vfio/pci.c | 5 ++-
hw/virtio/virtio-pci.c | 19 ++++++---
include/hw/pci/pci.h | 3 +-
include/hw/pci/pci_bridge.h | 3 +-
include/hw/pci/pcie.h | 3 +-
include/hw/pci/shpc.h | 3 +-
include/hw/pci/slotid_cap.h | 3 +-
21 files changed, 176 insertions(+), 131 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 1/7] pci: Clean up error checking in pci_add_capability()
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
@ 2017-06-06 11:26 ` Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 2/7] pci: Add comment for pci_add_capability2() Mao Zhongyi
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel
On success, pci_add_capability2() returns a positive value. On
failure, it sets an error and return a negative value.
pci_add_capability() laboriously checks this behavior. No other
caller does. Drop the checks from pci_add_capability().
Cc: mst@redhat.com
Cc: marcel@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/pci/pci.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 98ccc27..53566b8 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2270,12 +2270,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
Error *local_err = NULL;
ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
- if (local_err) {
- assert(ret < 0);
+ if (ret < 0) {
error_report_err(local_err);
- } else {
- /* success implies a positive offset in config space */
- assert(ret > 0);
}
return ret;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 2/7] pci: Add comment for pci_add_capability2()
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 1/7] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
@ 2017-06-06 11:26 ` Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 3/7] pci: Fix the return value checking Mao Zhongyi
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, armbru
Comments for pci_add_capability2() to explain the return
value. This may help to make a correct return value check
for its callers.
Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/pci/pci.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 53566b8..b73bfea 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2276,6 +2276,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
return ret;
}
+/*
+ * On success, pci_add_capability2() returns a positive value
+ * that the offset of the pci capability.
+ * On failure, it sets an error and returns a negative error
+ * code.
+ */
int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
Error **errp)
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 3/7] pci: Fix the return value checking
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 1/7] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 2/7] pci: Add comment for pci_add_capability2() Mao Zhongyi
@ 2017-06-06 11:26 ` Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 4/7] net/eepro100: Fix code style Mao Zhongyi
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel; +Cc: dmitry, jasowang, kraxel, alex.williamson, armbru
pci_add_capability returns a strictly positive value on success,
correct asserts.
Cc: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: kraxel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/net/e1000e.c | 2 +-
hw/net/eepro100.c | 2 +-
hw/usb/hcd-xhci.c | 2 +-
hw/vfio/pci.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 6e23493..8259d67 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -374,7 +374,7 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
{
int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
- if (ret >= 0) {
+ if (ret > 0) {
pci_set_word(pdev->config + offset + PCI_PM_PMC,
PCI_PM_CAP_VER_1_1 |
pmc);
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 4bf71f2..da36816 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -571,7 +571,7 @@ static void e100_pci_reset(EEPRO100State * s)
int cfg_offset = 0xdc;
int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
cfg_offset, PCI_PM_SIZEOF);
- assert(r >= 0);
+ assert(r > 0);
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
#if 0 /* TODO: replace dummy code for power management emulation. */
/* TODO: Power Management Control / Status. */
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index a0c7960..ab42f86 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3417,7 +3417,7 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
if (pci_bus_is_express(dev->bus) ||
xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
ret = pcie_endpoint_cap_init(dev, 0xa0);
- assert(ret >= 0);
+ assert(ret > 0);
}
if (xhci->msix != ON_OFF_AUTO_OFF) {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 32aca77..5881968 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1744,7 +1744,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
}
pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
- if (pos >= 0) {
+ if (pos > 0) {
vdev->pdev.exp.exp_cap = pos;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 4/7] net/eepro100: Fix code style
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
` (2 preceding siblings ...)
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 3/7] pci: Fix the return value checking Mao Zhongyi
@ 2017-06-06 11:26 ` Mao Zhongyi
2017-06-06 15:31 ` Michael S. Tsirkin
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, armbru
It reports a code style problem(ERROR: "foo * bar" should be "foo *bar")
when running checkpatch.pl. So fix it to conform to the coding standards.
Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/net/eepro100.c | 62 +++++++++++++++++++++++++++----------------------------
1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index da36816..62e989c 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -405,7 +405,7 @@ enum scb_stat_ack {
stat_ack_tx = (stat_ack_cu_idle | stat_ack_cu_cmd_done),
};
-static void disable_interrupt(EEPRO100State * s)
+static void disable_interrupt(EEPRO100State *s)
{
if (s->int_stat) {
TRACE(INT, logout("interrupt disabled\n"));
@@ -414,7 +414,7 @@ static void disable_interrupt(EEPRO100State * s)
}
}
-static void enable_interrupt(EEPRO100State * s)
+static void enable_interrupt(EEPRO100State *s)
{
if (!s->int_stat) {
TRACE(INT, logout("interrupt enabled\n"));
@@ -423,7 +423,7 @@ static void enable_interrupt(EEPRO100State * s)
}
}
-static void eepro100_acknowledge(EEPRO100State * s)
+static void eepro100_acknowledge(EEPRO100State *s)
{
s->scb_stat &= ~s->mem[SCBAck];
s->mem[SCBAck] = s->scb_stat;
@@ -432,7 +432,7 @@ static void eepro100_acknowledge(EEPRO100State * s)
}
}
-static void eepro100_interrupt(EEPRO100State * s, uint8_t status)
+static void eepro100_interrupt(EEPRO100State *s, uint8_t status)
{
uint8_t mask = ~s->mem[SCBIntmask];
s->mem[SCBAck] |= status;
@@ -449,52 +449,52 @@ static void eepro100_interrupt(EEPRO100State * s, uint8_t status)
}
}
-static void eepro100_cx_interrupt(EEPRO100State * s)
+static void eepro100_cx_interrupt(EEPRO100State *s)
{
/* CU completed action command. */
/* Transmit not ok (82557 only, not in emulation). */
eepro100_interrupt(s, 0x80);
}
-static void eepro100_cna_interrupt(EEPRO100State * s)
+static void eepro100_cna_interrupt(EEPRO100State *s)
{
/* CU left the active state. */
eepro100_interrupt(s, 0x20);
}
-static void eepro100_fr_interrupt(EEPRO100State * s)
+static void eepro100_fr_interrupt(EEPRO100State *s)
{
/* RU received a complete frame. */
eepro100_interrupt(s, 0x40);
}
-static void eepro100_rnr_interrupt(EEPRO100State * s)
+static void eepro100_rnr_interrupt(EEPRO100State *s)
{
/* RU is not ready. */
eepro100_interrupt(s, 0x10);
}
-static void eepro100_mdi_interrupt(EEPRO100State * s)
+static void eepro100_mdi_interrupt(EEPRO100State *s)
{
/* MDI completed read or write cycle. */
eepro100_interrupt(s, 0x08);
}
-static void eepro100_swi_interrupt(EEPRO100State * s)
+static void eepro100_swi_interrupt(EEPRO100State *s)
{
/* Software has requested an interrupt. */
eepro100_interrupt(s, 0x04);
}
#if 0
-static void eepro100_fcp_interrupt(EEPRO100State * s)
+static void eepro100_fcp_interrupt(EEPRO100State *s)
{
/* Flow control pause interrupt (82558 and later). */
eepro100_interrupt(s, 0x01);
}
#endif
-static void e100_pci_reset(EEPRO100State * s)
+static void e100_pci_reset(EEPRO100State *s)
{
E100PCIDeviceInfo *info = eepro100_get_class(s);
uint32_t device = s->device;
@@ -598,7 +598,7 @@ static void e100_pci_reset(EEPRO100State * s)
#endif /* EEPROM_SIZE > 0 */
}
-static void nic_selective_reset(EEPRO100State * s)
+static void nic_selective_reset(EEPRO100State *s)
{
size_t i;
uint16_t *eeprom_contents = eeprom93xx_data(s->eeprom);
@@ -669,7 +669,7 @@ static char *regname(uint32_t addr)
****************************************************************************/
#if 0
-static uint16_t eepro100_read_command(EEPRO100State * s)
+static uint16_t eepro100_read_command(EEPRO100State *s)
{
uint16_t val = 0xffff;
TRACE(OTHER, logout("val=0x%04x\n", val));
@@ -694,27 +694,27 @@ enum commands {
CmdTxFlex = 0x0008, /* Use "Flexible mode" for CmdTx command. */
};
-static cu_state_t get_cu_state(EEPRO100State * s)
+static cu_state_t get_cu_state(EEPRO100State *s)
{
return ((s->mem[SCBStatus] & BITS(7, 6)) >> 6);
}
-static void set_cu_state(EEPRO100State * s, cu_state_t state)
+static void set_cu_state(EEPRO100State *s, cu_state_t state)
{
s->mem[SCBStatus] = (s->mem[SCBStatus] & ~BITS(7, 6)) + (state << 6);
}
-static ru_state_t get_ru_state(EEPRO100State * s)
+static ru_state_t get_ru_state(EEPRO100State *s)
{
return ((s->mem[SCBStatus] & BITS(5, 2)) >> 2);
}
-static void set_ru_state(EEPRO100State * s, ru_state_t state)
+static void set_ru_state(EEPRO100State *s, ru_state_t state)
{
s->mem[SCBStatus] = (s->mem[SCBStatus] & ~BITS(5, 2)) + (state << 2);
}
-static void dump_statistics(EEPRO100State * s)
+static void dump_statistics(EEPRO100State *s)
{
/* Dump statistical data. Most data is never changed by the emulation
* and always 0, so we first just copy the whole block and then those
@@ -962,7 +962,7 @@ static void action_command(EEPRO100State *s)
/* List is empty. Now CU is idle or suspended. */
}
-static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
+static void eepro100_cu_command(EEPRO100State *s, uint8_t val)
{
cu_state_t cu_state;
switch (val) {
@@ -1036,7 +1036,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
}
}
-static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
+static void eepro100_ru_command(EEPRO100State *s, uint8_t val)
{
switch (val) {
case RU_NOP:
@@ -1084,7 +1084,7 @@ static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
}
}
-static void eepro100_write_command(EEPRO100State * s, uint8_t val)
+static void eepro100_write_command(EEPRO100State *s, uint8_t val)
{
eepro100_ru_command(s, val & 0x0f);
eepro100_cu_command(s, val & 0xf0);
@@ -1106,7 +1106,7 @@ static void eepro100_write_command(EEPRO100State * s, uint8_t val)
#define EEPROM_DI 0x04
#define EEPROM_DO 0x08
-static uint16_t eepro100_read_eeprom(EEPRO100State * s)
+static uint16_t eepro100_read_eeprom(EEPRO100State *s)
{
uint16_t val = e100_read_reg2(s, SCBeeprom);
if (eeprom93xx_read(s->eeprom)) {
@@ -1170,7 +1170,7 @@ static const char *reg2name(uint8_t reg)
}
#endif /* DEBUG_EEPRO100 */
-static uint32_t eepro100_read_mdi(EEPRO100State * s)
+static uint32_t eepro100_read_mdi(EEPRO100State *s)
{
uint32_t val = e100_read_reg4(s, SCBCtrlMDI);
@@ -1302,7 +1302,7 @@ typedef struct {
uint32_t st_result; /* Self Test Results */
} eepro100_selftest_t;
-static uint32_t eepro100_read_port(EEPRO100State * s)
+static uint32_t eepro100_read_port(EEPRO100State *s)
{
return 0;
}
@@ -1340,7 +1340,7 @@ static void eepro100_write_port(EEPRO100State *s)
*
****************************************************************************/
-static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr)
+static uint8_t eepro100_read1(EEPRO100State *s, uint32_t addr)
{
uint8_t val = 0;
if (addr <= sizeof(s->mem) - sizeof(val)) {
@@ -1393,7 +1393,7 @@ static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr)
return val;
}
-static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr)
+static uint16_t eepro100_read2(EEPRO100State *s, uint32_t addr)
{
uint16_t val = 0;
if (addr <= sizeof(s->mem) - sizeof(val)) {
@@ -1421,7 +1421,7 @@ static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr)
return val;
}
-static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
+static uint32_t eepro100_read4(EEPRO100State *s, uint32_t addr)
{
uint32_t val = 0;
if (addr <= sizeof(s->mem) - sizeof(val)) {
@@ -1453,7 +1453,7 @@ static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
return val;
}
-static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
+static void eepro100_write1(EEPRO100State *s, uint32_t addr, uint8_t val)
{
/* SCBStatus is readonly. */
if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
@@ -1519,7 +1519,7 @@ static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
}
}
-static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
+static void eepro100_write2(EEPRO100State *s, uint32_t addr, uint16_t val)
{
/* SCBStatus is readonly. */
if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
@@ -1565,7 +1565,7 @@ static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
}
}
-static void eepro100_write4(EEPRO100State * s, uint32_t addr, uint32_t val)
+static void eepro100_write4(EEPRO100State *s, uint32_t addr, uint32_t val)
{
if (addr <= sizeof(s->mem) - sizeof(val)) {
e100_write_reg4(s, addr, val);
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability()
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
` (3 preceding siblings ...)
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 4/7] net/eepro100: Fix code style Mao Zhongyi
@ 2017-06-06 11:26 ` Mao Zhongyi
2017-06-06 14:52 ` Eduardo Habkost
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 6/7] pci: Convert to realize Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 7/7] pci: Convert shpc_init() to Error Mao Zhongyi
6 siblings, 1 reply; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, rth, ehabkost, mst, dmitry, jasowang, marcel,
alex.williamson, armbru
Add Error argument for pci_add_capability() to leverage the errp
to pass info on errors. This way is helpful for its callers to
make a better error handling when moving to 'realize'.
Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
CC: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: marcel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
hw/net/e1000e.c | 7 ++++++-
hw/net/eepro100.c | 20 +++++++++++++++-----
hw/pci-bridge/i82801b11.c | 1 +
hw/pci/pci.c | 10 ++++------
hw/pci/pci_bridge.c | 7 ++++++-
hw/pci/pcie.c | 10 ++++++++--
hw/pci/shpc.c | 5 ++++-
hw/pci/slotid_cap.c | 7 ++++++-
hw/vfio/pci.c | 3 ++-
hw/virtio/virtio-pci.c | 19 ++++++++++++++-----
include/hw/pci/pci.h | 3 ++-
12 files changed, 85 insertions(+), 31 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 7b6d4ea..d93ffc2 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
x86_iommu->type = TYPE_AMD;
qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
- s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
- AMDVI_CAPAB_SIZE);
- assert(s->capab_offset > 0);
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
- assert(ret > 0);
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
- assert(ret > 0);
+ ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
+ AMDVI_CAPAB_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
+ s->capab_offset = ret;
+
+ ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
+ AMDVI_CAPAB_REG_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
+ ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
+ AMDVI_CAPAB_REG_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
/* set up MMIO */
memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 8259d67..41430766 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -47,6 +47,7 @@
#include "e1000e_core.h"
#include "trace.h"
+#include "qapi/error.h"
#define TYPE_E1000E "e1000e"
#define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
@@ -372,7 +373,9 @@ e1000e_gen_dsn(uint8_t *mac)
static int
e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
{
- int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
+ Error *local_err = NULL;
+ int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
+ PCI_PM_SIZEOF, &local_err);
if (ret > 0) {
pci_set_word(pdev->config + offset + PCI_PM_PMC,
@@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
PCI_PM_CTRL_PME_STATUS);
+ } else {
+ error_report_err(local_err);
}
return ret;
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 62e989c..0625839 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -48,6 +48,7 @@
#include "sysemu/sysemu.h"
#include "sysemu/dma.h"
#include "qemu/bitops.h"
+#include "qapi/error.h"
/* QEMU sends frames smaller than 60 bytes to ethernet nics.
* Such frames are rejected by real nics and their emulations.
@@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State *s)
}
#endif
-static void e100_pci_reset(EEPRO100State *s)
+static void e100_pci_reset(EEPRO100State *s, Error **errp)
{
E100PCIDeviceInfo *info = eepro100_get_class(s);
uint32_t device = s->device;
@@ -570,9 +571,13 @@ static void e100_pci_reset(EEPRO100State *s)
/* Power Management Capabilities */
int cfg_offset = 0xdc;
int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
- cfg_offset, PCI_PM_SIZEOF);
- assert(r > 0);
- pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+ cfg_offset, PCI_PM_SIZEOF,
+ errp);
+ if (r > 0) {
+ pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+ } else {
+ return;
+ }
#if 0 /* TODO: replace dummy code for power management emulation. */
/* TODO: Power Management Control / Status. */
pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
@@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
{
EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
E100PCIDeviceInfo *info = eepro100_get_class(s);
+ Error *local_err = NULL;
TRACE(OTHER, logout("\n"));
s->device = info->device;
- e100_pci_reset(s);
+ e100_pci_reset(s, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
/* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
* i82559 and later support 64 or 256 word EEPROM. */
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2404e7e..2c065c3 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -44,6 +44,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
#include "hw/i386/ich9.h"
+#include "qapi/error.h"
/*****************************************************************************/
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b73bfea..2bba37a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
* in pci config space
*/
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size)
+ uint8_t offset, uint8_t size,
+ Error **errp)
{
int ret;
- Error *local_err = NULL;
- ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
- if (ret < 0) {
- error_report_err(local_err);
- }
+ ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
+
return ret;
}
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 5118ef4..bb0f3a3 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -33,6 +33,7 @@
#include "hw/pci/pci_bridge.h"
#include "hw/pci/pci_bus.h"
#include "qemu/range.h"
+#include "qapi/error.h"
/* PCI bridge subsystem vendor ID helper functions */
#define PCI_SSVID_SIZEOF 8
@@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
uint16_t svid, uint16_t ssid)
{
int pos;
- pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
+ Error *local_err = NULL;
+
+ pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
+ PCI_SSVID_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 18e634f..f187512 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
/* PCIe cap v2 init */
int pos;
uint8_t *exp_cap;
+ Error *local_err = NULL;
assert(pci_is_express(dev));
- pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+ PCI_EXP_VER2_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
@@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
{
/* PCIe cap v1 init */
int pos;
+ Error *local_err = NULL;
assert(pci_is_express(dev));
- pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+ PCI_EXP_VER1_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 42fafac..d72d5e4 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
{
uint8_t *config;
int config_offset;
+ Error *local_err = NULL;
config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
- 0, SHPC_CAP_LENGTH);
+ 0, SHPC_CAP_LENGTH,
+ &local_err);
if (config_offset < 0) {
+ error_report_err(local_err);
return config_offset;
}
config = d->config + config_offset;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index aec1e91..bdca205 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -2,6 +2,7 @@
#include "hw/pci/slotid_cap.h"
#include "hw/pci/pci.h"
#include "qemu/error-report.h"
+#include "qapi/error.h"
#define SLOTID_CAP_LENGTH 4
#define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
@@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
unsigned offset)
{
int cap;
+ Error *local_err = NULL;
+
if (!chassis) {
error_report("Bridge chassis not specified. Each bridge is required "
"to be assigned a unique chassis id > 0.");
@@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
return -EINVAL;
}
- cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
+ cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
+ SLOTID_CAP_LENGTH, &local_err);
if (cap < 0) {
+ error_report_err(local_err);
return cap;
}
/* We make each chassis unique, this way each bridge is First in Chassis */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5881968..85cfe38 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1743,7 +1743,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
}
- pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
+ pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
+ errp);
if (pos > 0) {
vdev->pdev.exp.exp_cap = pos;
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9b7244..cca5276 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1161,9 +1161,14 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
{
PCIDevice *dev = &proxy->pci_dev;
int offset;
+ Error *local_err = NULL;
- offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
- assert(offset > 0);
+ offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
+ cap->cap_len, &local_err);
+ if (offset < 0) {
+ error_report_err(local_err);
+ abort();
+ }
assert(cap->cap_len >= sizeof *cap);
memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
@@ -1810,9 +1815,13 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
pos = pcie_endpoint_cap_init(pci_dev, 0);
assert(pos > 0);
- pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
- assert(pos > 0);
- pci_dev->exp.pm_cap = pos;
+ pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
+ PCI_PM_SIZEOF, errp);
+ if (pos > 0) {
+ pci_dev->exp.pm_cap = pos;
+ } else {
+ return;
+ }
/*
* Indicates that this function complies with revision 1.2 of the
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5..fe52aa8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size);
+ uint8_t offset, uint8_t size,
+ Error **errp);
int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
Error **errp);
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 6/7] pci: Convert to realize
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
` (4 preceding siblings ...)
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
@ 2017-06-06 11:26 ` Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 7/7] pci: Convert shpc_init() to Error Mao Zhongyi
6 siblings, 0 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, armbru
The pci-birdge device i82801b11 and io3130_upstream/downstream
still implements the old PCIDeviceClass .init() through *_init()
instead of the new .realize(). All devices need to be converted
to .realize(). So convert it and rename it to *_realize().
Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/pci-bridge/i82801b11.c | 11 +++++------
hw/pci-bridge/pcie_root_port.c | 15 ++++++---------
hw/pci-bridge/xio3130_downstream.c | 20 +++++++++-----------
hw/pci-bridge/xio3130_upstream.c | 20 +++++++++-----------
hw/pci/pci_bridge.c | 7 +++----
hw/pci/pcie.c | 11 ++++++-----
include/hw/pci/pci_bridge.h | 3 ++-
include/hw/pci/pcie.h | 3 ++-
8 files changed, 42 insertions(+), 48 deletions(-)
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2c065c3..2c1b747 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -59,24 +59,23 @@ typedef struct I82801b11Bridge {
/*< public >*/
} I82801b11Bridge;
-static int i82801b11_bridge_initfn(PCIDevice *d)
+static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
{
int rc;
pci_bridge_initfn(d, TYPE_PCI_BUS);
rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
- I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
+ I82801ba_SSVID_SVID, I82801ba_SSVID_SSID,
+ errp);
if (rc < 0) {
goto err_bridge;
}
pci_config_set_prog_interface(d->config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
- return 0;
+ return;
err_bridge:
pci_bridge_exitfn(d);
-
- return rc;
}
static const VMStateDescription i82801b11_bridge_dev_vmstate = {
@@ -96,7 +95,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
k->vendor_id = PCI_VENDOR_ID_INTEL;
k->device_id = PCI_DEVICE_ID_INTEL_82801BA_11;
k->revision = ICH9_D2P_A2_REVISION;
- k->init = i82801b11_bridge_initfn;
+ k->realize = i82801b11_bridge_realize;
k->config_write = pci_bridge_write_config;
dc->vmsd = &i82801b11_bridge_dev_vmstate;
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index cf36318..00f0b1f 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -59,29 +59,27 @@ static void rp_realize(PCIDevice *d, Error **errp)
PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
int rc;
- Error *local_err = NULL;
pci_config_set_interrupt_pin(d->config, 1);
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
- rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, rpc->ssid);
+ rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
+ rpc->ssid, errp);
if (rc < 0) {
- error_setg(errp, "Can't init SSV ID, error %d", rc);
goto err_bridge;
}
if (rpc->interrupts_init) {
- rc = rpc->interrupts_init(d, &local_err);
+ rc = rpc->interrupts_init(d, errp);
if (rc < 0) {
- error_propagate(errp, local_err);
goto err_bridge;
}
}
- rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT, p->port);
+ rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
+ p->port, errp);
if (rc < 0) {
- error_setg(errp, "Can't add Root Port capability, error %d", rc);
goto err_int;
}
@@ -98,9 +96,8 @@ static void rp_realize(PCIDevice *d, Error **errp)
}
rc = pcie_aer_init(d, PCI_ERR_VER, rpc->aer_offset,
- PCI_ERR_SIZEOF, &local_err);
+ PCI_ERR_SIZEOF, errp);
if (rc < 0) {
- error_propagate(errp, local_err);
goto err;
}
pcie_aer_root_init(d);
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index cfe8a36..e706f36 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
pci_bridge_reset(qdev);
}
-static int xio3130_downstream_initfn(PCIDevice *d)
+static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
{
PCIEPort *p = PCIE_PORT(d);
PCIESlot *s = PCIE_SLOT(d);
int rc;
- Error *err = NULL;
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
- XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
+ XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+ errp);
if (rc < 0) {
assert(rc == -ENOTSUP);
- error_report_err(err);
goto err_bridge;
}
rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
- XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
+ XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
+ errp);
if (rc < 0) {
goto err_bridge;
}
rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
- p->port);
+ p->port, errp);
if (rc < 0) {
goto err_msi;
}
@@ -98,13 +98,12 @@ static int xio3130_downstream_initfn(PCIDevice *d)
}
rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
- PCI_ERR_SIZEOF, &err);
+ PCI_ERR_SIZEOF, errp);
if (rc < 0) {
- error_report_err(err);
goto err;
}
- return 0;
+ return;
err:
pcie_chassis_del_slot(s);
@@ -114,7 +113,6 @@ err_msi:
msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
- return rc;
}
static void xio3130_downstream_exitfn(PCIDevice *d)
@@ -181,7 +179,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
k->is_express = 1;
k->is_bridge = 1;
k->config_write = xio3130_downstream_write_config;
- k->init = xio3130_downstream_initfn;
+ k->realize = xio3130_downstream_realize;
k->exit = xio3130_downstream_exitfn;
k->vendor_id = PCI_VENDOR_ID_TI;
k->device_id = PCI_DEVICE_ID_TI_XIO3130D;
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 401c784..a052224 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -53,32 +53,32 @@ static void xio3130_upstream_reset(DeviceState *qdev)
pcie_cap_deverr_reset(d);
}
-static int xio3130_upstream_initfn(PCIDevice *d)
+static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
{
PCIEPort *p = PCIE_PORT(d);
int rc;
- Error *err = NULL;
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
- XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
+ XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+ errp);
if (rc < 0) {
assert(rc == -ENOTSUP);
- error_report_err(err);
goto err_bridge;
}
rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
- XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
+ XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
+ errp);
if (rc < 0) {
goto err_bridge;
}
rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
- p->port);
+ p->port, errp);
if (rc < 0) {
goto err_msi;
}
@@ -86,13 +86,12 @@ static int xio3130_upstream_initfn(PCIDevice *d)
pcie_cap_deverr_init(d);
rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
- PCI_ERR_SIZEOF, &err);
+ PCI_ERR_SIZEOF, errp);
if (rc < 0) {
- error_report_err(err);
goto err;
}
- return 0;
+ return;
err:
pcie_cap_exit(d);
@@ -100,7 +99,6 @@ err_msi:
msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
- return rc;
}
static void xio3130_upstream_exitfn(PCIDevice *d)
@@ -153,7 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass, void *data)
k->is_express = 1;
k->is_bridge = 1;
k->config_write = xio3130_upstream_write_config;
- k->init = xio3130_upstream_initfn;
+ k->realize = xio3130_upstream_realize;
k->exit = xio3130_upstream_exitfn;
k->vendor_id = PCI_VENDOR_ID_TI;
k->device_id = PCI_DEVICE_ID_TI_XIO3130U;
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index bb0f3a3..720119b 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -41,15 +41,14 @@
#define PCI_SSVID_SSID 6
int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
- uint16_t svid, uint16_t ssid)
+ uint16_t svid, uint16_t ssid,
+ Error **errp)
{
int pos;
- Error *local_err = NULL;
pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
- PCI_SSVID_SIZEOF, &local_err);
+ PCI_SSVID_SIZEOF, errp);
if (pos < 0) {
- error_report_err(local_err);
return pos;
}
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index f187512..05d091a 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -86,19 +86,19 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
pci_set_word(cmask + PCI_EXP_LNKSTA, 0);
}
-int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
+int pcie_cap_init(PCIDevice *dev, uint8_t offset,
+ uint8_t type, uint8_t port,
+ Error **errp)
{
/* PCIe cap v2 init */
int pos;
uint8_t *exp_cap;
- Error *local_err = NULL;
assert(pci_is_express(dev));
pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
- PCI_EXP_VER2_SIZEOF, &local_err);
+ PCI_EXP_VER2_SIZEOF, errp);
if (pos < 0) {
- error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
@@ -147,6 +147,7 @@ static int
pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
{
uint8_t type = PCI_EXP_TYPE_ENDPOINT;
+ Error *local_err = NULL;
/*
* Windows guests will report Code 10, device cannot start, if
@@ -159,7 +160,7 @@ pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
return (cap_size == PCI_EXP_VER1_SIZEOF)
? pcie_cap_v1_init(dev, offset, type, 0)
- : pcie_cap_init(dev, offset, type, 0);
+ : pcie_cap_init(dev, offset, type, 0, &local_err);
}
int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index d5891cd..ff7cbaa 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -33,7 +33,8 @@
#define PCI_BRIDGE_DEV_PROP_SHPC "shpc"
int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
- uint16_t svid, uint16_t ssid);
+ uint16_t svid, uint16_t ssid,
+ Error **errp);
PCIDevice *pci_bridge_get_device(PCIBus *bus);
PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 3d8f24b..b71e369 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -84,7 +84,8 @@ struct PCIExpressDevice {
#define COMPAT_PROP_PCP "power_controller_present"
/* PCI express capability helper functions */
-int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
+int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
+ uint8_t port, Error **errp);
int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
uint8_t type, uint8_t port);
int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 7/7] pci: Convert shpc_init() to Error
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
` (5 preceding siblings ...)
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 6/7] pci: Convert to realize Mao Zhongyi
@ 2017-06-06 11:26 ` Mao Zhongyi
6 siblings, 0 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-06 11:26 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, armbru
In order to propagate error message better, convert shpc_init() to
Error also convert the pci_bridge_dev_initfn() to realize.
Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
hw/pci/shpc.c | 11 +++++------
hw/pci/slotid_cap.c | 11 +++++------
include/hw/pci/shpc.h | 3 ++-
include/hw/pci/slotid_cap.h | 3 ++-
5 files changed, 22 insertions(+), 27 deletions(-)
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 5dbd933..30c4186 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -49,12 +49,11 @@ struct PCIBridgeDev {
};
typedef struct PCIBridgeDev PCIBridgeDev;
-static int pci_bridge_dev_initfn(PCIDevice *dev)
+static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
{
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);
@@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
dev->config[PCI_INTERRUPT_PIN] = 0x1;
memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
shpc_bar_size(dev));
- err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
+ err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
if (err) {
goto shpc_error;
}
@@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
bridge_dev->msi = ON_OFF_AUTO_OFF;
}
- err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
+ err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
if (err) {
goto slotid_error;
}
@@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
/* it means SHPC exists, because MSI is needed by SHPC */
- err = msi_init(dev, 0, 1, true, true, &local_err);
+ err = msi_init(dev, 0, 1, true, true, errp);
/* Any error other than -ENOTSUP(board's MSI support is broken)
* is a programming error */
assert(!err || err == -ENOTSUP);
if (err && bridge_dev->msi == ON_OFF_AUTO_ON) {
/* Can't satisfy user's explicit msi=on request, fail */
- error_append_hint(&local_err, "You have to use msi=auto (default) "
+ error_append_hint(errp, "You have to use msi=auto (default) "
"or msi=off with this machine type.\n");
- error_report_err(local_err);
goto msi_error;
}
- assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO);
+ assert(bridge_dev->msi == ON_OFF_AUTO_AUTO);
/* With msi=auto, we fall back to MSI off silently */
- error_free(local_err);
}
if (shpc_present(dev)) {
@@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
}
- return 0;
+ return;
msi_error:
slotid_cap_cleanup(dev);
@@ -111,8 +108,6 @@ slotid_error:
}
shpc_error:
pci_bridge_exitfn(dev);
-
- return err;
}
static void pci_bridge_dev_exitfn(PCIDevice *dev)
@@ -216,7 +211,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
- k->init = pci_bridge_dev_initfn;
+ k->realize = pci_bridge_dev_realize;
k->exit = pci_bridge_dev_exitfn;
k->config_write = pci_bridge_dev_write_config;
k->vendor_id = PCI_VENDOR_ID_REDHAT;
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index d72d5e4..69fc14b 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
}
/* Add SHPC capability to the config space for the device. */
-static int shpc_cap_add_config(PCIDevice *d)
+static int shpc_cap_add_config(PCIDevice *d, Error **errp)
{
uint8_t *config;
int config_offset;
- Error *local_err = NULL;
config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
0, SHPC_CAP_LENGTH,
- &local_err);
+ errp);
if (config_offset < 0) {
- error_report_err(local_err);
return config_offset;
}
config = d->config + config_offset;
@@ -584,13 +582,14 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
}
/* Initialize the SHPC structure in bridge's BAR. */
-int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
+int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
+ unsigned offset, Error **errp)
{
int i, ret;
int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
shpc->sec_bus = sec_bus;
- ret = shpc_cap_add_config(d);
+ ret = shpc_cap_add_config(d, errp);
if (ret) {
g_free(d->shpc);
return ret;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index bdca205..36d021b 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -9,14 +9,14 @@
int slotid_cap_init(PCIDevice *d, int nslots,
uint8_t chassis,
- unsigned offset)
+ unsigned offset,
+ Error **errp)
{
int cap;
- Error *local_err = NULL;
if (!chassis) {
- error_report("Bridge chassis not specified. Each bridge is required "
- "to be assigned a unique chassis id > 0.");
+ error_setg(errp, "Bridge chassis not specified. Each bridge is required"
+ " to be assigned a unique chassis id > 0.");
return -EINVAL;
}
if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >> SLOTID_NSLOTS_SHIFT)) {
@@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
}
cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
- SLOTID_CAP_LENGTH, &local_err);
+ SLOTID_CAP_LENGTH, errp);
if (cap < 0) {
- error_report_err(local_err);
return cap;
}
/* We make each chassis unique, this way each bridge is First in Chassis */
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index b208554..1ea88b1 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -39,7 +39,8 @@ struct SHPCDevice {
void shpc_reset(PCIDevice *d);
int shpc_bar_size(PCIDevice *dev);
-int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
+int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
+ unsigned off, Error **errp);
void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
void shpc_free(PCIDevice *dev);
void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
index 70db047..a777ea0 100644
--- a/include/hw/pci/slotid_cap.h
+++ b/include/hw/pci/slotid_cap.h
@@ -5,7 +5,8 @@
int slotid_cap_init(PCIDevice *dev, int nslots,
uint8_t chassis,
- unsigned offset);
+ unsigned offset,
+ Error **errp);
void slotid_cap_cleanup(PCIDevice *dev);
#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability()
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
@ 2017-06-06 14:52 ` Eduardo Habkost
2017-06-06 16:24 ` Markus Armbruster
2017-06-07 5:33 ` Mao Zhongyi
0 siblings, 2 replies; 15+ messages in thread
From: Eduardo Habkost @ 2017-06-06 14:52 UTC (permalink / raw)
To: Mao Zhongyi
Cc: qemu-devel, pbonzini, rth, mst, dmitry, jasowang, marcel,
alex.williamson, armbru
On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
> Add Error argument for pci_add_capability() to leverage the errp
> to pass info on errors. This way is helpful for its callers to
> make a better error handling when moving to 'realize'.
>
> Cc: pbonzini@redhat.com
> Cc: rth@twiddle.net
> Cc: ehabkost@redhat.com
> Cc: mst@redhat.com
> CC: dmitry@daynix.com
> Cc: jasowang@redhat.com
> Cc: marcel@redhat.com
> Cc: alex.williamson@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
> hw/net/e1000e.c | 7 ++++++-
> hw/net/eepro100.c | 20 +++++++++++++++-----
> hw/pci-bridge/i82801b11.c | 1 +
> hw/pci/pci.c | 10 ++++------
> hw/pci/pci_bridge.c | 7 ++++++-
> hw/pci/pcie.c | 10 ++++++++--
> hw/pci/shpc.c | 5 ++++-
> hw/pci/slotid_cap.c | 7 ++++++-
> hw/vfio/pci.c | 3 ++-
> hw/virtio/virtio-pci.c | 19 ++++++++++++++-----
> include/hw/pci/pci.h | 3 ++-
> 12 files changed, 85 insertions(+), 31 deletions(-)
There are multiple places below that checks for errors like this:
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
}
Sometimes it even includes another branch for the error path:
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
} else {
/* error path here */
return ret;
}
I suggest doing this instead, for readability:
function(...)
if (function failed) {
return ...; /* or: "goto out" */
}
/* non-error code path here */
foo = bar;
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 7b6d4ea..d93ffc2 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
> x86_iommu->type = TYPE_AMD;
> qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
> object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
> - s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> - AMDVI_CAPAB_SIZE);
> - assert(s->capab_offset > 0);
> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
> - assert(ret > 0);
> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
> - assert(ret > 0);
> + ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> + AMDVI_CAPAB_SIZE, err);
> + if (ret < 0) {
> + return;
Maybe adding a local_err variable is preferred instead of
checking for (pos < 0), but I'm not sure it's necessary.
Markus, what's the recommendation on those cases? Should we use
the negative return value to avoid adding an extra local_err
variable, or should we add local_err anyway to match the existing
style elsewhere?
(The same applies to other functions below [*])
> + }
> + s->capab_offset = ret;
> +
> + ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
> + AMDVI_CAPAB_REG_SIZE, err);
> + if (ret < 0) {
> + return;
> + }
> + ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
> + AMDVI_CAPAB_REG_SIZE, err);
> + if (ret < 0) {
> + return;
> + }
>
> /* set up MMIO */
> memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 8259d67..41430766 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -47,6 +47,7 @@
> #include "e1000e_core.h"
>
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define TYPE_E1000E "e1000e"
> #define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
> @@ -372,7 +373,9 @@ e1000e_gen_dsn(uint8_t *mac)
> static int
> e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
> {
> - int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
> + Error *local_err = NULL;
> + int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
> + PCI_PM_SIZEOF, &local_err);
>
> if (ret > 0) {
> pci_set_word(pdev->config + offset + PCI_PM_PMC,
> @@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
>
> pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
> PCI_PM_CTRL_PME_STATUS);
> + } else {
> + error_report_err(local_err);
> }
I suggest this instead:
int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
PCI_PM_SIZEOF, &local_err);
if (local_err) {
error_report_err(local_err);
return ret;
}
pci_set_word(...);
pci_set_word(...);
pci_set_word(...);
return ret;
>
> return ret;
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 62e989c..0625839 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -48,6 +48,7 @@
> #include "sysemu/sysemu.h"
> #include "sysemu/dma.h"
> #include "qemu/bitops.h"
> +#include "qapi/error.h"
>
> /* QEMU sends frames smaller than 60 bytes to ethernet nics.
> * Such frames are rejected by real nics and their emulations.
> @@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State *s)
> }
> #endif
>
> -static void e100_pci_reset(EEPRO100State *s)
> +static void e100_pci_reset(EEPRO100State *s, Error **errp)
> {
> E100PCIDeviceInfo *info = eepro100_get_class(s);
> uint32_t device = s->device;
> @@ -570,9 +571,13 @@ static void e100_pci_reset(EEPRO100State *s)
> /* Power Management Capabilities */
> int cfg_offset = 0xdc;
> int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
> - cfg_offset, PCI_PM_SIZEOF);
> - assert(r > 0);
> - pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> + cfg_offset, PCI_PM_SIZEOF,
> + errp);
> + if (r > 0) {
> + pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> + } else {
> + return;
> + }
I suggest this instead:
int r = pci_add_capability(..., errp);
if (r < 0) { [*]
return;
}
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
...
Or, even better, as this function is very long:
Error *local_err = NULL;
pci_add_capability(..., &local_err);
if (local_err) {
goto out;
}
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
...
out:
error_propagate(errp, local_err);
return;
> #if 0 /* TODO: replace dummy code for power management emulation. */
> /* TODO: Power Management Control / Status. */
> pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
> @@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
> {
> EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
> E100PCIDeviceInfo *info = eepro100_get_class(s);
> + Error *local_err = NULL;
>
> TRACE(OTHER, logout("\n"));
>
> s->device = info->device;
>
> - e100_pci_reset(s);
> + e100_pci_reset(s, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
>
> /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
> * i82559 and later support 64 or 256 word EEPROM. */
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index 2404e7e..2c065c3 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -44,6 +44,7 @@
> #include "qemu/osdep.h"
> #include "hw/pci/pci.h"
> #include "hw/i386/ich9.h"
> +#include "qapi/error.h"
>
>
> /*****************************************************************************/
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b73bfea..2bba37a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
> * in pci config space
> */
> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size)
> + uint8_t offset, uint8_t size,
> + Error **errp)
> {
> int ret;
> - Error *local_err = NULL;
>
> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
> - if (ret < 0) {
> - error_report_err(local_err);
> - }
> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
> +
> return ret;
> }
pci_add_capability() and pci_add_capability2() now do exactly the
same, why are both being kept? I suggest replacing
pci_add_capability2() with pci_add_capability() everywhere (on a
separate patch).
>
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 5118ef4..bb0f3a3 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -33,6 +33,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "hw/pci/pci_bus.h"
> #include "qemu/range.h"
> +#include "qapi/error.h"
>
> /* PCI bridge subsystem vendor ID helper functions */
> #define PCI_SSVID_SIZEOF 8
> @@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
> uint16_t svid, uint16_t ssid)
> {
> int pos;
> - pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
> + Error *local_err = NULL;
> +
> + pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
> + PCI_SSVID_SIZEOF, &local_err);
> if (pos < 0) {
[*]
> + error_report_err(local_err);
> return pos;
> }
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 18e634f..f187512 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
> /* PCIe cap v2 init */
> int pos;
> uint8_t *exp_cap;
> + Error *local_err = NULL;
>
> assert(pci_is_express(dev));
>
> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> + PCI_EXP_VER2_SIZEOF, &local_err);
> if (pos < 0) {
[*]
> + error_report_err(local_err);
> return pos;
> }
> dev->exp.exp_cap = pos;
> @@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
> {
> /* PCIe cap v1 init */
> int pos;
> + Error *local_err = NULL;
>
> assert(pci_is_express(dev));
>
> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> + PCI_EXP_VER1_SIZEOF, &local_err);
> if (pos < 0) {
[*]
> + error_report_err(local_err);
> return pos;
> }
> dev->exp.exp_cap = pos;
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 42fafac..d72d5e4 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
> {
> uint8_t *config;
> int config_offset;
> + Error *local_err = NULL;
> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
> - 0, SHPC_CAP_LENGTH);
> + 0, SHPC_CAP_LENGTH,
> + &local_err);
> if (config_offset < 0) {
[*]
> + error_report_err(local_err);
> return config_offset;
> }
> config = d->config + config_offset;
> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
> index aec1e91..bdca205 100644
> --- a/hw/pci/slotid_cap.c
> +++ b/hw/pci/slotid_cap.c
> @@ -2,6 +2,7 @@
> #include "hw/pci/slotid_cap.h"
> #include "hw/pci/pci.h"
> #include "qemu/error-report.h"
> +#include "qapi/error.h"
>
> #define SLOTID_CAP_LENGTH 4
> #define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
> @@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
> unsigned offset)
> {
> int cap;
> + Error *local_err = NULL;
> +
> if (!chassis) {
> error_report("Bridge chassis not specified. Each bridge is required "
> "to be assigned a unique chassis id > 0.");
> @@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
> return -EINVAL;
> }
>
> - cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
> + cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
> + SLOTID_CAP_LENGTH, &local_err);
> if (cap < 0) {
[*]
> + error_report_err(local_err);
> return cap;
> }
> /* We make each chassis unique, this way each bridge is First in Chassis */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5881968..85cfe38 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1743,7 +1743,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
> PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
> }
>
> - pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
> + pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
> + errp);
> if (pos > 0) {
> vdev->pdev.exp.exp_cap = pos;
> }
I would this instead:
pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
errp);
if (pos < 0) { [*]
return pos;
}
vdev->pdev.exp.exp_cap = pos;
...
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..cca5276 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1161,9 +1161,14 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
> {
> PCIDevice *dev = &proxy->pci_dev;
> int offset;
> + Error *local_err = NULL;
>
> - offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
> - assert(offset > 0);
> + offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
> + cap->cap_len, &local_err);
> + if (offset < 0) {
> + error_report_err(local_err);
> + abort();
> + }
>
This can be simplified to:
offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
cap->cap_len, &error_abort);
> assert(cap->cap_len >= sizeof *cap);
> memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> @@ -1810,9 +1815,13 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> pos = pcie_endpoint_cap_init(pci_dev, 0);
> assert(pos > 0);
>
> - pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
> - assert(pos > 0);
> - pci_dev->exp.pm_cap = pos;
> + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
> + PCI_PM_SIZEOF, errp);
> + if (pos > 0) {
> + pci_dev->exp.pm_cap = pos;
> + } else {
> + return;
> + }
>
I suggest:
pos = pci_add_capability(...)
if (pos < 0) {
return;
}
pci_dev->exp.pm_cap = pos;
> /*
> * Indicates that this function complies with revision 1.2 of the
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..fe52aa8 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
> pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
>
> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size);
> + uint8_t offset, uint8_t size,
> + Error **errp);
> int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
> uint8_t offset, uint8_t size,
> Error **errp);
> --
> 2.9.3
>
>
>
--
Eduardo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/7] net/eepro100: Fix code style
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 4/7] net/eepro100: Fix code style Mao Zhongyi
@ 2017-06-06 15:31 ` Michael S. Tsirkin
2017-06-07 2:43 ` Mao Zhongyi
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2017-06-06 15:31 UTC (permalink / raw)
To: Mao Zhongyi; +Cc: qemu-devel, jasowang, armbru
On Tue, Jun 06, 2017 at 07:26:29PM +0800, Mao Zhongyi wrote:
> It reports a code style problem(ERROR: "foo * bar" should be "foo *bar")
> when running checkpatch.pl. So fix it to conform to the coding standards.
>
> Cc: jasowang@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
We don't generally do this kind of drive-by coding cleanups.
Wait until you actually make some changes to this file.
I'm also not merging this through the pci tree, pls split this out.
> ---
> hw/net/eepro100.c | 62 +++++++++++++++++++++++++++----------------------------
> 1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index da36816..62e989c 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -405,7 +405,7 @@ enum scb_stat_ack {
> stat_ack_tx = (stat_ack_cu_idle | stat_ack_cu_cmd_done),
> };
>
> -static void disable_interrupt(EEPRO100State * s)
> +static void disable_interrupt(EEPRO100State *s)
> {
> if (s->int_stat) {
> TRACE(INT, logout("interrupt disabled\n"));
> @@ -414,7 +414,7 @@ static void disable_interrupt(EEPRO100State * s)
> }
> }
>
> -static void enable_interrupt(EEPRO100State * s)
> +static void enable_interrupt(EEPRO100State *s)
> {
> if (!s->int_stat) {
> TRACE(INT, logout("interrupt enabled\n"));
> @@ -423,7 +423,7 @@ static void enable_interrupt(EEPRO100State * s)
> }
> }
>
> -static void eepro100_acknowledge(EEPRO100State * s)
> +static void eepro100_acknowledge(EEPRO100State *s)
> {
> s->scb_stat &= ~s->mem[SCBAck];
> s->mem[SCBAck] = s->scb_stat;
> @@ -432,7 +432,7 @@ static void eepro100_acknowledge(EEPRO100State * s)
> }
> }
>
> -static void eepro100_interrupt(EEPRO100State * s, uint8_t status)
> +static void eepro100_interrupt(EEPRO100State *s, uint8_t status)
> {
> uint8_t mask = ~s->mem[SCBIntmask];
> s->mem[SCBAck] |= status;
> @@ -449,52 +449,52 @@ static void eepro100_interrupt(EEPRO100State * s, uint8_t status)
> }
> }
>
> -static void eepro100_cx_interrupt(EEPRO100State * s)
> +static void eepro100_cx_interrupt(EEPRO100State *s)
> {
> /* CU completed action command. */
> /* Transmit not ok (82557 only, not in emulation). */
> eepro100_interrupt(s, 0x80);
> }
>
> -static void eepro100_cna_interrupt(EEPRO100State * s)
> +static void eepro100_cna_interrupt(EEPRO100State *s)
> {
> /* CU left the active state. */
> eepro100_interrupt(s, 0x20);
> }
>
> -static void eepro100_fr_interrupt(EEPRO100State * s)
> +static void eepro100_fr_interrupt(EEPRO100State *s)
> {
> /* RU received a complete frame. */
> eepro100_interrupt(s, 0x40);
> }
>
> -static void eepro100_rnr_interrupt(EEPRO100State * s)
> +static void eepro100_rnr_interrupt(EEPRO100State *s)
> {
> /* RU is not ready. */
> eepro100_interrupt(s, 0x10);
> }
>
> -static void eepro100_mdi_interrupt(EEPRO100State * s)
> +static void eepro100_mdi_interrupt(EEPRO100State *s)
> {
> /* MDI completed read or write cycle. */
> eepro100_interrupt(s, 0x08);
> }
>
> -static void eepro100_swi_interrupt(EEPRO100State * s)
> +static void eepro100_swi_interrupt(EEPRO100State *s)
> {
> /* Software has requested an interrupt. */
> eepro100_interrupt(s, 0x04);
> }
>
> #if 0
> -static void eepro100_fcp_interrupt(EEPRO100State * s)
> +static void eepro100_fcp_interrupt(EEPRO100State *s)
> {
> /* Flow control pause interrupt (82558 and later). */
> eepro100_interrupt(s, 0x01);
> }
> #endif
>
> -static void e100_pci_reset(EEPRO100State * s)
> +static void e100_pci_reset(EEPRO100State *s)
> {
> E100PCIDeviceInfo *info = eepro100_get_class(s);
> uint32_t device = s->device;
> @@ -598,7 +598,7 @@ static void e100_pci_reset(EEPRO100State * s)
> #endif /* EEPROM_SIZE > 0 */
> }
>
> -static void nic_selective_reset(EEPRO100State * s)
> +static void nic_selective_reset(EEPRO100State *s)
> {
> size_t i;
> uint16_t *eeprom_contents = eeprom93xx_data(s->eeprom);
> @@ -669,7 +669,7 @@ static char *regname(uint32_t addr)
> ****************************************************************************/
>
> #if 0
> -static uint16_t eepro100_read_command(EEPRO100State * s)
> +static uint16_t eepro100_read_command(EEPRO100State *s)
> {
> uint16_t val = 0xffff;
> TRACE(OTHER, logout("val=0x%04x\n", val));
> @@ -694,27 +694,27 @@ enum commands {
> CmdTxFlex = 0x0008, /* Use "Flexible mode" for CmdTx command. */
> };
>
> -static cu_state_t get_cu_state(EEPRO100State * s)
> +static cu_state_t get_cu_state(EEPRO100State *s)
> {
> return ((s->mem[SCBStatus] & BITS(7, 6)) >> 6);
> }
>
> -static void set_cu_state(EEPRO100State * s, cu_state_t state)
> +static void set_cu_state(EEPRO100State *s, cu_state_t state)
> {
> s->mem[SCBStatus] = (s->mem[SCBStatus] & ~BITS(7, 6)) + (state << 6);
> }
>
> -static ru_state_t get_ru_state(EEPRO100State * s)
> +static ru_state_t get_ru_state(EEPRO100State *s)
> {
> return ((s->mem[SCBStatus] & BITS(5, 2)) >> 2);
> }
>
> -static void set_ru_state(EEPRO100State * s, ru_state_t state)
> +static void set_ru_state(EEPRO100State *s, ru_state_t state)
> {
> s->mem[SCBStatus] = (s->mem[SCBStatus] & ~BITS(5, 2)) + (state << 2);
> }
>
> -static void dump_statistics(EEPRO100State * s)
> +static void dump_statistics(EEPRO100State *s)
> {
> /* Dump statistical data. Most data is never changed by the emulation
> * and always 0, so we first just copy the whole block and then those
> @@ -962,7 +962,7 @@ static void action_command(EEPRO100State *s)
> /* List is empty. Now CU is idle or suspended. */
> }
>
> -static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
> +static void eepro100_cu_command(EEPRO100State *s, uint8_t val)
> {
> cu_state_t cu_state;
> switch (val) {
> @@ -1036,7 +1036,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
> }
> }
>
> -static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
> +static void eepro100_ru_command(EEPRO100State *s, uint8_t val)
> {
> switch (val) {
> case RU_NOP:
> @@ -1084,7 +1084,7 @@ static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
> }
> }
>
> -static void eepro100_write_command(EEPRO100State * s, uint8_t val)
> +static void eepro100_write_command(EEPRO100State *s, uint8_t val)
> {
> eepro100_ru_command(s, val & 0x0f);
> eepro100_cu_command(s, val & 0xf0);
> @@ -1106,7 +1106,7 @@ static void eepro100_write_command(EEPRO100State * s, uint8_t val)
> #define EEPROM_DI 0x04
> #define EEPROM_DO 0x08
>
> -static uint16_t eepro100_read_eeprom(EEPRO100State * s)
> +static uint16_t eepro100_read_eeprom(EEPRO100State *s)
> {
> uint16_t val = e100_read_reg2(s, SCBeeprom);
> if (eeprom93xx_read(s->eeprom)) {
> @@ -1170,7 +1170,7 @@ static const char *reg2name(uint8_t reg)
> }
> #endif /* DEBUG_EEPRO100 */
>
> -static uint32_t eepro100_read_mdi(EEPRO100State * s)
> +static uint32_t eepro100_read_mdi(EEPRO100State *s)
> {
> uint32_t val = e100_read_reg4(s, SCBCtrlMDI);
>
> @@ -1302,7 +1302,7 @@ typedef struct {
> uint32_t st_result; /* Self Test Results */
> } eepro100_selftest_t;
>
> -static uint32_t eepro100_read_port(EEPRO100State * s)
> +static uint32_t eepro100_read_port(EEPRO100State *s)
> {
> return 0;
> }
> @@ -1340,7 +1340,7 @@ static void eepro100_write_port(EEPRO100State *s)
> *
> ****************************************************************************/
>
> -static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr)
> +static uint8_t eepro100_read1(EEPRO100State *s, uint32_t addr)
> {
> uint8_t val = 0;
> if (addr <= sizeof(s->mem) - sizeof(val)) {
> @@ -1393,7 +1393,7 @@ static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr)
> return val;
> }
>
> -static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr)
> +static uint16_t eepro100_read2(EEPRO100State *s, uint32_t addr)
> {
> uint16_t val = 0;
> if (addr <= sizeof(s->mem) - sizeof(val)) {
> @@ -1421,7 +1421,7 @@ static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr)
> return val;
> }
>
> -static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
> +static uint32_t eepro100_read4(EEPRO100State *s, uint32_t addr)
> {
> uint32_t val = 0;
> if (addr <= sizeof(s->mem) - sizeof(val)) {
> @@ -1453,7 +1453,7 @@ static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
> return val;
> }
>
> -static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
> +static void eepro100_write1(EEPRO100State *s, uint32_t addr, uint8_t val)
> {
> /* SCBStatus is readonly. */
> if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
> @@ -1519,7 +1519,7 @@ static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
> }
> }
>
> -static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
> +static void eepro100_write2(EEPRO100State *s, uint32_t addr, uint16_t val)
> {
> /* SCBStatus is readonly. */
> if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
> @@ -1565,7 +1565,7 @@ static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
> }
> }
>
> -static void eepro100_write4(EEPRO100State * s, uint32_t addr, uint32_t val)
> +static void eepro100_write4(EEPRO100State *s, uint32_t addr, uint32_t val)
> {
> if (addr <= sizeof(s->mem) - sizeof(val)) {
> e100_write_reg4(s, addr, val);
> --
> 2.9.3
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability()
2017-06-06 14:52 ` Eduardo Habkost
@ 2017-06-06 16:24 ` Markus Armbruster
2017-06-07 5:33 ` Mao Zhongyi
1 sibling, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-06-06 16:24 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Mao Zhongyi, mst, jasowang, qemu-devel, marcel, alex.williamson,
dmitry, pbonzini, rth
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>> Add Error argument for pci_add_capability() to leverage the errp
>> to pass info on errors. This way is helpful for its callers to
>> make a better error handling when moving to 'realize'.
>>
>> Cc: pbonzini@redhat.com
>> Cc: rth@twiddle.net
>> Cc: ehabkost@redhat.com
>> Cc: mst@redhat.com
>> CC: dmitry@daynix.com
>> Cc: jasowang@redhat.com
>> Cc: marcel@redhat.com
>> Cc: alex.williamson@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
>> hw/net/e1000e.c | 7 ++++++-
>> hw/net/eepro100.c | 20 +++++++++++++++-----
>> hw/pci-bridge/i82801b11.c | 1 +
>> hw/pci/pci.c | 10 ++++------
>> hw/pci/pci_bridge.c | 7 ++++++-
>> hw/pci/pcie.c | 10 ++++++++--
>> hw/pci/shpc.c | 5 ++++-
>> hw/pci/slotid_cap.c | 7 ++++++-
>> hw/vfio/pci.c | 3 ++-
>> hw/virtio/virtio-pci.c | 19 ++++++++++++++-----
>> include/hw/pci/pci.h | 3 ++-
>> 12 files changed, 85 insertions(+), 31 deletions(-)
>
>
> There are multiple places below that checks for errors like this:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> }
>
> Sometimes it even includes another branch for the error path:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> } else {
> /* error path here */
> return ret;
> }
>
> I suggest doing this instead, for readability:
>
> function(...)
> if (function failed) {
> return ...; /* or: "goto out" */
> }
>
> /* non-error code path here */
> foo = bar;
Yes, please.
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 7b6d4ea..d93ffc2 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
>> x86_iommu->type = TYPE_AMD;
>> qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
>> object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
>> - s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
>> - AMDVI_CAPAB_SIZE);
>> - assert(s->capab_offset > 0);
>> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
>> - assert(ret > 0);
>> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
>> - assert(ret > 0);
>> + ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
>> + AMDVI_CAPAB_SIZE, err);
>> + if (ret < 0) {
>> + return;
>
> Maybe adding a local_err variable is preferred instead of
> checking for (pos < 0), but I'm not sure it's necessary.
>
> Markus, what's the recommendation on those cases? Should we use
> the negative return value to avoid adding an extra local_err
> variable, or should we add local_err anyway to match the existing
> style elsewhere?
Opinions and practice vary on this one.
I prefer checking the return value because it lets me avoid the
error_propagate() boiler-plate more often.
Having both an Error parameter and an error return value poses the
question whether the two agree.
You can assert they do, but it's distracting. We generally don't.
When there's no success value to transmit, you avoid the problem by
making the function return void. We used to favor that, but it has
turned out not to be a success, because it leads to cumbersome code.
For what it's worth, GLib wants you to transmit success / failure in the
return value, too:
https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html#gerror-rules
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/7] net/eepro100: Fix code style
2017-06-06 15:31 ` Michael S. Tsirkin
@ 2017-06-07 2:43 ` Mao Zhongyi
0 siblings, 0 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-07 2:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, jasowang, armbru
Hi, Michael
On 06/06/2017 11:31 PM, Michael S. Tsirkin wrote:
> On Tue, Jun 06, 2017 at 07:26:29PM +0800, Mao Zhongyi wrote:
>> It reports a code style problem(ERROR: "foo * bar" should be "foo *bar")
>> when running checkpatch.pl. So fix it to conform to the coding standards.
>>
>> Cc: jasowang@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>
> We don't generally do this kind of drive-by coding cleanups.
> Wait until you actually make some changes to this file.
> I'm also not merging this through the pci tree, pls split this out.
>
The e100_pci_reset() in this file will be modified in the patch5. it will
reports a code style problem such as 'ERROR: "foo * bar" should be "foo *bar"'
when executing the checkpatch.pl for patch5.
Of course, I could simply modify the e100_pci_reset() to avoid this error,
but I think it's not a good idea. Since fix, all the same error should be
fixed absolutely.
So I make a separate patch to fix it, meanwhile prepare for patch5.
Thanks
Mao
>> ---
>> hw/net/eepro100.c | 62 +++++++++++++++++++++++++++----------------------------
>> 1 file changed, 31 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>> index da36816..62e989c 100644
>> --- a/hw/net/eepro100.c
>> +++ b/hw/net/eepro100.c
>> @@ -405,7 +405,7 @@ enum scb_stat_ack {
>> stat_ack_tx = (stat_ack_cu_idle | stat_ack_cu_cmd_done),
>> };
>>
>> -static void disable_interrupt(EEPRO100State * s)
>> +static void disable_interrupt(EEPRO100State *s)
>> {
>> if (s->int_stat) {
>> TRACE(INT, logout("interrupt disabled\n"));
>> @@ -414,7 +414,7 @@ static void disable_interrupt(EEPRO100State * s)
>> }
>> }
>>
>> -static void enable_interrupt(EEPRO100State * s)
>> +static void enable_interrupt(EEPRO100State *s)
>> {
>> if (!s->int_stat) {
>> TRACE(INT, logout("interrupt enabled\n"));
>> @@ -423,7 +423,7 @@ static void enable_interrupt(EEPRO100State * s)
>> }
>> }
>>
>> -static void eepro100_acknowledge(EEPRO100State * s)
>> +static void eepro100_acknowledge(EEPRO100State *s)
>> {
>> s->scb_stat &= ~s->mem[SCBAck];
>> s->mem[SCBAck] = s->scb_stat;
>> @@ -432,7 +432,7 @@ static void eepro100_acknowledge(EEPRO100State * s)
>> }
>> }
>>
>> -static void eepro100_interrupt(EEPRO100State * s, uint8_t status)
>> +static void eepro100_interrupt(EEPRO100State *s, uint8_t status)
>> {
>> uint8_t mask = ~s->mem[SCBIntmask];
>> s->mem[SCBAck] |= status;
>> @@ -449,52 +449,52 @@ static void eepro100_interrupt(EEPRO100State * s, uint8_t status)
>> }
>> }
>>
>> -static void eepro100_cx_interrupt(EEPRO100State * s)
>> +static void eepro100_cx_interrupt(EEPRO100State *s)
>> {
>> /* CU completed action command. */
>> /* Transmit not ok (82557 only, not in emulation). */
>> eepro100_interrupt(s, 0x80);
>> }
>>
>> -static void eepro100_cna_interrupt(EEPRO100State * s)
>> +static void eepro100_cna_interrupt(EEPRO100State *s)
>> {
>> /* CU left the active state. */
>> eepro100_interrupt(s, 0x20);
>> }
>>
>> -static void eepro100_fr_interrupt(EEPRO100State * s)
>> +static void eepro100_fr_interrupt(EEPRO100State *s)
>> {
>> /* RU received a complete frame. */
>> eepro100_interrupt(s, 0x40);
>> }
>>
>> -static void eepro100_rnr_interrupt(EEPRO100State * s)
>> +static void eepro100_rnr_interrupt(EEPRO100State *s)
>> {
>> /* RU is not ready. */
>> eepro100_interrupt(s, 0x10);
>> }
>>
>> -static void eepro100_mdi_interrupt(EEPRO100State * s)
>> +static void eepro100_mdi_interrupt(EEPRO100State *s)
>> {
>> /* MDI completed read or write cycle. */
>> eepro100_interrupt(s, 0x08);
>> }
>>
>> -static void eepro100_swi_interrupt(EEPRO100State * s)
>> +static void eepro100_swi_interrupt(EEPRO100State *s)
>> {
>> /* Software has requested an interrupt. */
>> eepro100_interrupt(s, 0x04);
>> }
>>
>> #if 0
>> -static void eepro100_fcp_interrupt(EEPRO100State * s)
>> +static void eepro100_fcp_interrupt(EEPRO100State *s)
>> {
>> /* Flow control pause interrupt (82558 and later). */
>> eepro100_interrupt(s, 0x01);
>> }
>> #endif
>>
>> -static void e100_pci_reset(EEPRO100State * s)
>> +static void e100_pci_reset(EEPRO100State *s)
>> {
>> E100PCIDeviceInfo *info = eepro100_get_class(s);
>> uint32_t device = s->device;
>> @@ -598,7 +598,7 @@ static void e100_pci_reset(EEPRO100State * s)
>> #endif /* EEPROM_SIZE > 0 */
>> }
>>
>> -static void nic_selective_reset(EEPRO100State * s)
>> +static void nic_selective_reset(EEPRO100State *s)
>> {
>> size_t i;
>> uint16_t *eeprom_contents = eeprom93xx_data(s->eeprom);
>> @@ -669,7 +669,7 @@ static char *regname(uint32_t addr)
>> ****************************************************************************/
>>
>> #if 0
>> -static uint16_t eepro100_read_command(EEPRO100State * s)
>> +static uint16_t eepro100_read_command(EEPRO100State *s)
>> {
>> uint16_t val = 0xffff;
>> TRACE(OTHER, logout("val=0x%04x\n", val));
>> @@ -694,27 +694,27 @@ enum commands {
>> CmdTxFlex = 0x0008, /* Use "Flexible mode" for CmdTx command. */
>> };
>>
>> -static cu_state_t get_cu_state(EEPRO100State * s)
>> +static cu_state_t get_cu_state(EEPRO100State *s)
>> {
>> return ((s->mem[SCBStatus] & BITS(7, 6)) >> 6);
>> }
>>
>> -static void set_cu_state(EEPRO100State * s, cu_state_t state)
>> +static void set_cu_state(EEPRO100State *s, cu_state_t state)
>> {
>> s->mem[SCBStatus] = (s->mem[SCBStatus] & ~BITS(7, 6)) + (state << 6);
>> }
>>
>> -static ru_state_t get_ru_state(EEPRO100State * s)
>> +static ru_state_t get_ru_state(EEPRO100State *s)
>> {
>> return ((s->mem[SCBStatus] & BITS(5, 2)) >> 2);
>> }
>>
>> -static void set_ru_state(EEPRO100State * s, ru_state_t state)
>> +static void set_ru_state(EEPRO100State *s, ru_state_t state)
>> {
>> s->mem[SCBStatus] = (s->mem[SCBStatus] & ~BITS(5, 2)) + (state << 2);
>> }
>>
>> -static void dump_statistics(EEPRO100State * s)
>> +static void dump_statistics(EEPRO100State *s)
>> {
>> /* Dump statistical data. Most data is never changed by the emulation
>> * and always 0, so we first just copy the whole block and then those
>> @@ -962,7 +962,7 @@ static void action_command(EEPRO100State *s)
>> /* List is empty. Now CU is idle or suspended. */
>> }
>>
>> -static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
>> +static void eepro100_cu_command(EEPRO100State *s, uint8_t val)
>> {
>> cu_state_t cu_state;
>> switch (val) {
>> @@ -1036,7 +1036,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
>> }
>> }
>>
>> -static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
>> +static void eepro100_ru_command(EEPRO100State *s, uint8_t val)
>> {
>> switch (val) {
>> case RU_NOP:
>> @@ -1084,7 +1084,7 @@ static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
>> }
>> }
>>
>> -static void eepro100_write_command(EEPRO100State * s, uint8_t val)
>> +static void eepro100_write_command(EEPRO100State *s, uint8_t val)
>> {
>> eepro100_ru_command(s, val & 0x0f);
>> eepro100_cu_command(s, val & 0xf0);
>> @@ -1106,7 +1106,7 @@ static void eepro100_write_command(EEPRO100State * s, uint8_t val)
>> #define EEPROM_DI 0x04
>> #define EEPROM_DO 0x08
>>
>> -static uint16_t eepro100_read_eeprom(EEPRO100State * s)
>> +static uint16_t eepro100_read_eeprom(EEPRO100State *s)
>> {
>> uint16_t val = e100_read_reg2(s, SCBeeprom);
>> if (eeprom93xx_read(s->eeprom)) {
>> @@ -1170,7 +1170,7 @@ static const char *reg2name(uint8_t reg)
>> }
>> #endif /* DEBUG_EEPRO100 */
>>
>> -static uint32_t eepro100_read_mdi(EEPRO100State * s)
>> +static uint32_t eepro100_read_mdi(EEPRO100State *s)
>> {
>> uint32_t val = e100_read_reg4(s, SCBCtrlMDI);
>>
>> @@ -1302,7 +1302,7 @@ typedef struct {
>> uint32_t st_result; /* Self Test Results */
>> } eepro100_selftest_t;
>>
>> -static uint32_t eepro100_read_port(EEPRO100State * s)
>> +static uint32_t eepro100_read_port(EEPRO100State *s)
>> {
>> return 0;
>> }
>> @@ -1340,7 +1340,7 @@ static void eepro100_write_port(EEPRO100State *s)
>> *
>> ****************************************************************************/
>>
>> -static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr)
>> +static uint8_t eepro100_read1(EEPRO100State *s, uint32_t addr)
>> {
>> uint8_t val = 0;
>> if (addr <= sizeof(s->mem) - sizeof(val)) {
>> @@ -1393,7 +1393,7 @@ static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr)
>> return val;
>> }
>>
>> -static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr)
>> +static uint16_t eepro100_read2(EEPRO100State *s, uint32_t addr)
>> {
>> uint16_t val = 0;
>> if (addr <= sizeof(s->mem) - sizeof(val)) {
>> @@ -1421,7 +1421,7 @@ static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr)
>> return val;
>> }
>>
>> -static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
>> +static uint32_t eepro100_read4(EEPRO100State *s, uint32_t addr)
>> {
>> uint32_t val = 0;
>> if (addr <= sizeof(s->mem) - sizeof(val)) {
>> @@ -1453,7 +1453,7 @@ static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
>> return val;
>> }
>>
>> -static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
>> +static void eepro100_write1(EEPRO100State *s, uint32_t addr, uint8_t val)
>> {
>> /* SCBStatus is readonly. */
>> if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
>> @@ -1519,7 +1519,7 @@ static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
>> }
>> }
>>
>> -static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
>> +static void eepro100_write2(EEPRO100State *s, uint32_t addr, uint16_t val)
>> {
>> /* SCBStatus is readonly. */
>> if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
>> @@ -1565,7 +1565,7 @@ static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
>> }
>> }
>>
>> -static void eepro100_write4(EEPRO100State * s, uint32_t addr, uint32_t val)
>> +static void eepro100_write4(EEPRO100State *s, uint32_t addr, uint32_t val)
>> {
>> if (addr <= sizeof(s->mem) - sizeof(val)) {
>> e100_write_reg4(s, addr, val);
>> --
>> 2.9.3
>>
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability()
2017-06-06 14:52 ` Eduardo Habkost
2017-06-06 16:24 ` Markus Armbruster
@ 2017-06-07 5:33 ` Mao Zhongyi
2017-06-07 7:05 ` Markus Armbruster
1 sibling, 1 reply; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-07 5:33 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, pbonzini, rth, mst, dmitry, jasowang, marcel,
alex.williamson, armbru
Hi, Eduardo
On 06/06/2017 10:52 PM, Eduardo Habkost wrote:
> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>> Add Error argument for pci_add_capability() to leverage the errp
>> to pass info on errors. This way is helpful for its callers to
>> make a better error handling when moving to 'realize'.
>>
>> Cc: pbonzini@redhat.com
>> Cc: rth@twiddle.net
>> Cc: ehabkost@redhat.com
>> Cc: mst@redhat.com
>> CC: dmitry@daynix.com
>> Cc: jasowang@redhat.com
>> Cc: marcel@redhat.com
>> Cc: alex.williamson@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
[...]
>
>
> There are multiple places below that checks for errors like this:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> }
>
> Sometimes it even includes another branch for the error path:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> } else {
> /* error path here */
> return ret;
> }
>
> I suggest doing this instead, for readability:
>
> function(...)
> if (function failed) {
> return ...; /* or: "goto out" */
> }
>
> /* non-error code path here */
> foo = bar;
>
Thank you very much for the detailed explanation，will use
this more elegant way to check return value in next version. :)
[...]
>> static int
>> e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
>> {
>> - int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
>> + Error *local_err = NULL;
>> + int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
>> + PCI_PM_SIZEOF, &local_err);
>>
>> if (ret > 0) {
>> pci_set_word(pdev->config + offset + PCI_PM_PMC,
>> @@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
>>
>> pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
>> PCI_PM_CTRL_PME_STATUS);
>> + } else {
>> + error_report_err(local_err);
>> }
>
>
> I suggest this instead:
>
> int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
> PCI_PM_SIZEOF, &local_err);
> if (local_err) {
> error_report_err(local_err);
> return ret;
> }
>
> pci_set_word(...);
> pci_set_word(...);
> pci_set_word(...);
> return ret;
>
OK, I see.
>
>>
>>
>> /*****************************************************************************/
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index b73bfea..2bba37a 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
>> * in pci config space
>> */
>> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>> - uint8_t offset, uint8_t size)
>> + uint8_t offset, uint8_t size,
>> + Error **errp)
>> {
>> int ret;
>> - Error *local_err = NULL;
>>
>> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
>> - if (ret < 0) {
>> - error_report_err(local_err);
>> - }
>> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
>> +
>> return ret;
>> }
>
> pci_add_capability() and pci_add_capability2() now do exactly the
> same, why are both being kept? I suggest replacing
> pci_add_capability2() with pci_add_capability() everywhere (on a
> separate patch).
>
Completely remove pci_add_capability and direct use pci_add_capability2()
everywhere is it a more thorough way?
Thanks
Mao
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability()
2017-06-07 5:33 ` Mao Zhongyi
@ 2017-06-07 7:05 ` Markus Armbruster
2017-06-07 9:33 ` Mao Zhongyi
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2017-06-07 7:05 UTC (permalink / raw)
To: Mao Zhongyi
Cc: Eduardo Habkost, mst, jasowang, qemu-devel, marcel,
alex.williamson, dmitry, pbonzini, rth
Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> Hi, Eduardo
>
> On 06/06/2017 10:52 PM, Eduardo Habkost wrote:
>> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>>> Add Error argument for pci_add_capability() to leverage the errp
>>> to pass info on errors. This way is helpful for its callers to
>>> make a better error handling when moving to 'realize'.
[...]
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index b73bfea..2bba37a 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
>>> * in pci config space
>>> */
>>> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>>> - uint8_t offset, uint8_t size)
>>> + uint8_t offset, uint8_t size,
>>> + Error **errp)
>>> {
>>> int ret;
>>> - Error *local_err = NULL;
>>>
>>> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
>>> - if (ret < 0) {
>>> - error_report_err(local_err);
>>> - }
>>> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
>>> +
>>> return ret;
>>> }
>>
>> pci_add_capability() and pci_add_capability2() now do exactly the
>> same, why are both being kept? I suggest replacing
>> pci_add_capability2() with pci_add_capability() everywhere (on a
>> separate patch).
>>
>
> Completely remove pci_add_capability and direct use pci_add_capability2()
> everywhere is it a more thorough way?
You're converting pci_add_capability() to Error because you need the
Error for your conversions to realize().
I recommend to change the calls where you need the Error (and only
these) to call pci_add_capability2() instead.
When no calls to pci_add_capability() remain, we remove it. If that
becomes the case in your series, you remove it.
Okay?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability()
2017-06-07 7:05 ` Markus Armbruster
@ 2017-06-07 9:33 ` Mao Zhongyi
0 siblings, 0 replies; 15+ messages in thread
From: Mao Zhongyi @ 2017-06-07 9:33 UTC (permalink / raw)
To: Markus Armbruster
Cc: Eduardo Habkost, mst, jasowang, qemu-devel, marcel,
alex.williamson, dmitry, pbonzini, rth
Hi, Markus
On 06/07/2017 03:05 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Hi, Eduardo
>>
>> On 06/06/2017 10:52 PM, Eduardo Habkost wrote:
>>> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>>>> Add Error argument for pci_add_capability() to leverage the errp
>>>> to pass info on errors. This way is helpful for its callers to
>>>> make a better error handling when moving to 'realize'.
> [...]
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index b73bfea..2bba37a 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
>>>> * in pci config space
>>>> */
>>>> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>>>> - uint8_t offset, uint8_t size)
>>>> + uint8_t offset, uint8_t size,
>>>> + Error **errp)
>>>> {
>>>> int ret;
>>>> - Error *local_err = NULL;
>>>>
>>>> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
>>>> - if (ret < 0) {
>>>> - error_report_err(local_err);
>>>> - }
>>>> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
>>>> +
>>>> return ret;
>>>> }
>>>
>>> pci_add_capability() and pci_add_capability2() now do exactly the
>>> same, why are both being kept? I suggest replacing
>>> pci_add_capability2() with pci_add_capability() everywhere (on a
>>> separate patch).
>>>
>>
>> Completely remove pci_add_capability and direct use pci_add_capability2()
>> everywhere is it a more thorough way?
>
> You're converting pci_add_capability() to Error because you need the
> Error for your conversions to realize().
it's true.
>
> I recommend to change the calls where you need the Error (and only
> these) to call pci_add_capability2() instead.
>
> When no calls to pci_add_capability() remain, we remove it. If that
> becomes the case in your series, you remove it.
>
> Okay?
This is a gentle way of doing it. After read the code I found only
parts need to be replaced by pci_add_capability2() in my series as
follow your advice, this is no problem. But it means that the remaining
replacement will be reworked in the future, although it can be fixed
absolutely in a separate patch now. Of course, this is just my own
opinion, consider the reason for git history I would rather hear your
advice. :)
Thanks
Mao
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-06-07 9:34 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-06 11:26 [Qemu-devel] [PATCH v3 0/7] Convert to realize and cleanup Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 1/7] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 2/7] pci: Add comment for pci_add_capability2() Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 3/7] pci: Fix the return value checking Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 4/7] net/eepro100: Fix code style Mao Zhongyi
2017-06-06 15:31 ` Michael S. Tsirkin
2017-06-07 2:43 ` Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
2017-06-06 14:52 ` Eduardo Habkost
2017-06-06 16:24 ` Markus Armbruster
2017-06-07 5:33 ` Mao Zhongyi
2017-06-07 7:05 ` Markus Armbruster
2017-06-07 9:33 ` Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 6/7] pci: Convert to realize Mao Zhongyi
2017-06-06 11:26 ` [Qemu-devel] [PATCH v3 7/7] pci: Convert shpc_init() to Error Mao Zhongyi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).