Re: [PATCH V7 06/10] powerpc/eeh: Create PE for VFs

[Wei Yang <weiyang@linux.vnet.ibm.com>]

On Wed, Jun 03, 2015 at 10:46:38AM -0500, Bjorn Helgaas wrote:
>On Wed, Jun 03, 2015 at 03:10:23PM +1000, Gavin Shan wrote:
>> 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.
>
>> >>> +	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().
>> >
>> >>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()?

>Bjorn

-- 
Richard Yang
Help you, Help me