On 08/07/18 14:19, Laszlo Ersek wrote: > On 08/07/18 09:04, Jing Liu wrote: >> Add hint to firmware (e.g. SeaBIOS) to reserve addtional >> IO/MEM/PREF spaces for legacy pci-pci bridge, to enable >> some pci devices hotplugging whose IO/MEM/PREF spaces >> requests are larger than the ones in pci-pci bridge set >> by firmware. >> >> Signed-off-by: Jing Liu <jing2....@linux.intel.com> >> --- >> hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+)
>> +cap_error: >> + msi_uninit(dev); > > (4) This error handler doesn't look entirely correct; we can reach it > without having initialized MSI. (MSI setup is conditional; and even if > we attempt it, it is permitted to fail with "msi=auto".) Therefore, > msi_uninit() should be wrapped into "if (msi_present())", same as you > see in pci_bridge_dev_exitfn(). > >> msi_error: >> slotid_cap_cleanup(dev); >> slotid_error: I tried to understand the error handling a bit better. I'm confused. First, under the label "shpc_error", we call pci_bridge_exitfn(), which seems to clean up everything (checking individually for each thing to clean up). Given this, I wonder why we introduced the "slotid_error" and "msi_error" labels at all. Cascading teardown on the error path, and invkoing a function that checks everything individually and then tears it all down, are usually mutually exclusive. Second, msi_uninit() and shpc_cleanup() are internally inconsistent between each other. The former removes the respective capability from config space -- with pci_del_capability() --, but the latter only has a comment, "/* TODO: cleanup config space changes? */". The same comment is present in the slotid_cap_cleanup() function. Given this inconsistency, I don't know what to suggest for the new capability's teardown, in pci_bridge_dev_exitfn() -- should we just ignore it (as suggested by this patch)? Thanks Laszlo
