On 06/22/2017 02:29 PM, Marcel Apfelbaum wrote:
On 22/06/2017 6:47, Mao Zhongyi wrote:On 06/21/2017 03:39 PM, Marcel Apfelbaum wrote:On 20/06/2017 6:51, Mao Zhongyi wrote:Hi, Marcel On 06/19/2017 08:05 PM, Marcel Apfelbaum wrote:On 12/06/2017 16:48, Mao Zhongyi wrote:In order to propagate error message better, convert shpc_init() to Error also convert the pci_bridge_dev_initfn() to realize. Cc: m...@redhat.com Cc: mar...@redhat.com Cc: arm...@redhat.com Signed-off-by: Mao Zhongyi <maozy.f...@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);I am not sure we can drop the !local_err assert. We don't have a local_err anymore, but the error is stored now in errp.I think it's OK if we drop the !local_err assert; even if we don't store the error message to errp. Because the code can go to assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO) only if the return value of msi_init is 0(success), then the local_err is NULL,I think I am missing something. What if msi_init returns with -ENOTSUP and bridge_dev->msi == ON_OFF_AUTO_AUTO ? We do reach the assert with an err. Previously it was a asserted "error allowed only on auto mode", now we assert only "auto mode".Hi, Marcel Thanks for your explanation, you are right, I am missing something. We can do reach the assert in the next three cases: ------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | | previous | now | | | case | condition |-------------------------------------------------------------|---------------------------------------------| result | | | | assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | | |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------| | | err == -ENOTSUP | local_err != NULL, is equivalent to: | | | | 1 | bridge_dev->msi == ON_OFF_AUTO_AUTO | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | same | |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------| | | err == 0 | local_err == NULL, is equivalent to: | | | | 2 | bridge_dev->msi == ON_OFF_AUTO_AUTO | assert(!local_err) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | same | |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------| | | err == 0 | local_err == NULL, is equivalent to: | | | | 3 | bridge_dev->msi != ON_OFF_AUTO_AUTO | assert(!local_err) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) |different| ------------------------------------------------------------------------------------------------------------------------------------------------------------------ In the case 1 & 2, the code runs the same path as before, but in case 3 does not, it will be abort after the patch, So drop the !local_err assert is really not appropriate, will revert it in the v7. :)Thanks for looking into it and sorry for the late review. Marcel
That's all right. :) Mao
Thanks, Marcel noerror message in it. So the assertation of local_err is always superfluous.Thanks MaoOther than that, the patch looks OK to me. Thanks, Marcel/* 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 71e836b..ee19fec 100644 --- a/include/hw/pci/shpc.h +++ b/include/hw/pci/shpc.h @@ -38,7 +38,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