qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge
Date: Fri, 17 Aug 2018 18:32:47 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1



On 08/14/2018 12:15 PM, Liu, Jing2 wrote:
Hi Marcel,

On 8/12/2018 3:11 PM, Marcel Apfelbaum wrote:
Hi Laszlo,

[...]
  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().

You are right.  msi_present should be checked.

I looked at the codes calling this function. It need be added to be strong.
But could I ask why we need check twice? msi_unint() help check again.

You are right, no need to check again. Sorry for misleading you.



  msi_error:
      slotid_cap_cleanup(dev);
  slotid_error:
I tried to understand the error handling a bit better. I'm confused.

[...]
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() --,

Right

  but the latter only has a
comment, "/* TODO: cleanup config space changes? */".

But also disables the QEMU_PCI_CAP_SLOTID (but no code checks it anyway)
I agree it should call pci_del_capability to delete the SHPC capability,
maybe is some "debt" from early development stages.

  The same comment
is present in the slotid_cap_cleanup() function. Given this
inconsistency,

Here we also miss a call to pci_del_capability.

I don't know what to suggest for the new capability's
teardown, in pci_bridge_dev_exitfn()
Aha, yes, the teardown needs to be added here.
Will add that in next version.

-- should we just ignore it (as
suggested by this patch)?

No, we should remove it properly.

I think it is not considered a "big" issue since adding/removing PCI capabilities is not an operation that deals with resources, we only edit an array (the config space)
that will not be used anyway if the device init sequence failed.

That does not mean the code should not be clean.
Do we need set up another serial patches (separated from this
one) to add pci_del_capability() for slotid_cap_cleanup() and shpc_cleanup()?


It would be nice, but as you pointed out, it doesn't have to be part
of this series.

Thanks,
Marcel

Thanks,
Jing

[...]




reply via email to

[Prev in Thread] Current Thread [Next in Thread]