On Tue, Jun 16, 2015 at 3:50 AM, Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx> wrote: > 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()? I would expect it to be called from pci_bus_add_device(). Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
