On Tue, Jun 16, 2015 at 3:50 AM, Wei Yang [off-list ref] wrote:

On Wed, Jun 03, 2015 at 10:46:38AM -0500, Bjorn Helgaas wrote:

quoted

On Wed, Jun 03, 2015 at 03:10:23PM +1000, Gavin Shan wrote:

quoted

It's correct. "The following operations" refers to eeh_add_device_late()
and eeh_sysfs_add_device(). The former one requires the resources for
one particular PCI device (VF here) are finalized (assigned). eeh_sysfs_add_device()
will fail if the sysfs entry for the PCI device isn't populated yet.

eeh_add_device_late() contains several things that read config space:
eeh_save_bars() caches the entire config header, and
eeh_addr_cache_insert_dev() looks at the device resources (which are
determined by BARs in config space).  I think this is an error-prone
approach.  I think it would be simpler and safer for you to capture what
you need in your PCI config accessors.

eeh_add_device_late() also contains code to deal with an EEH cache that
"might not be removed correctly because of unbalanced kref to the device
during unplug time."  That's unrelated to this patch series, but it sounds
... like a hacky workaround for some bug in the unplug path.

quoted

quoted

quoted

quoted

+        eeh_add_device_early(pdn);
+        eeh_add_device_late(pdev);
+        eeh_sysfs_add_device(pdev);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pnv_eeh_vf_final_fixup);

Ugh.  This is powerpc code, but I don't like using fixups as a hook like
this.  There is a pcibios_add_device() -- could this be done there?

I don't like it neither :-) But looks we can't put it in the
pcibios_add_device().

quoted

If not, what happens after pcibios_add_device() that is required for this
code?  Maybe we need a pcibios_bus_add_device() hook?

The pnv_eeh_vf_final_fixup() will try to create sysfs for VFs. This requires
the VF sysfs(kobj) is initialized properly. If we put these into
pcibios_add_device(), the eeh_sysfs_add_device() would fail.

Below is the call flow for your reference:

pci_device_add()
   pcibios_add_device()
   device_add()                <--- kobj initialized here

We can put it into pcibios_bus_add_device(), but we don't it currently. If
Bjorn agree to add pcibios_bus_add_device(), I'm fine to move the block code
there.

I think I'm OK with adding a pcibios_bus_add_device().  I think that would
be better than using the fixup mechanism for this.

Hi, Bjorn, Gavin,

Been working on some bug recently, just got a chance to this one.

Would you mind giving me some hint, where you suggest to put the
pcibios_bus_add_device()?

I would expect it to be called from pci_bus_add_device().

Bjorn