Oliver O'Halloran <ooh...@gmail.com> writes:
> The pnv_phb->initialized flag is an odd beast. It was added back in 2012 in
> commit db1266c85261 ("powerpc/powernv: Skip check on PE if necessary") to
> allow devices to be enabled even if their PE assignments hadn't been
> completed yet. I can't think of any situation where we would (or should)
> have PCI devices being enabled before their PEs are assigned, so I can only
> assume it was a workaround for a bug or some other undesirable behaviour
> from the PCI core.
>
> Since commit dc3d8f85bb57 ("powerpc/powernv/pci: Re-work bus PE
> configuration") the PE setup occurs before the PCI core allows driver to
> attach to the device so the problem should no longer exist. Even it does
> allowing the device to be enabled before we have assigned the device to a
> PE is almost certainly broken and will cause spurious EEH events so we
> should probably just remove it.
>
> It's also worth pointing out that ->initialized flag is set in
> pnv_pci_ioda_create_dbgfs() which has the entire function body wrapped in
> flag.
"body wrapped in flag." ?
I guess you meant:
"wrapped in #ifdef CONFIG_DEBUG_FS" ?
> That has the fun side effect of bypassing any other checks in
> pnv_pci_enable_device_hook() which is probably not what anybody wants.
That would only be true for CONFIG_DEBUG_FS=n builds though.
cheers
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 023a4f987bb2..6ac3c637b313 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -2410,9 +2410,6 @@ static void pnv_pci_ioda_create_dbgfs(void)
> list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
> phb = hose->private_data;
>
> - /* Notify initialization of PHB done */
> - phb->initialized = 1;
> -
> sprintf(name, "PCI%04x", hose->global_number);
> phb->dbgfs = debugfs_create_dir(name, powerpc_debugfs_root);
>
> @@ -2609,17 +2606,8 @@ static resource_size_t pnv_pci_default_alignment(void)
> */
> static bool pnv_pci_enable_device_hook(struct pci_dev *dev)
> {
> - struct pnv_phb *phb = pci_bus_to_pnvhb(dev->bus);
> struct pci_dn *pdn;
>
> - /* The function is probably called while the PEs have
> - * not be created yet. For example, resource reassignment
> - * during PCI probe period. We just skip the check if
> - * PEs isn't ready.
> - */
> - if (!phb->initialized)
> - return true;
> -
> pdn = pci_get_pdn(dev);
> if (!pdn || pdn->pe_number == IODA_INVALID_PE)
> return false;
> @@ -2629,14 +2617,9 @@ static bool pnv_pci_enable_device_hook(struct pci_dev
> *dev)
>
> static bool pnv_ocapi_enable_device_hook(struct pci_dev *dev)
> {
> - struct pci_controller *hose = pci_bus_to_host(dev->bus);
> - struct pnv_phb *phb = hose->private_data;
> struct pci_dn *pdn;
> struct pnv_ioda_pe *pe;
>
> - if (!phb->initialized)
> - return true;
> -
> pdn = pci_get_pdn(dev);
> if (!pdn)
> return false;
> diff --git a/arch/powerpc/platforms/powernv/pci.h
> b/arch/powerpc/platforms/powernv/pci.h
> index 739a0b3b72e1..36d22920f5a3 100644
> --- a/arch/powerpc/platforms/powernv/pci.h
> +++ b/arch/powerpc/platforms/powernv/pci.h
> @@ -119,7 +119,6 @@ struct pnv_phb {
> int flags;
> void __iomem *regs;
> u64 regs_phys;
> - int initialized;
> spinlock_t lock;
>
> #ifdef CONFIG_DEBUG_FS
> --
> 2.26.2
