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 -- 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
