[PATCH] powerpc/pseries/iommu: DLPAR ADD of pci device doesn't completely initialize pci_controller structure

[Nathan Lynch]

Hi Gaurav,

A couple minor comments below.

Gaurav Batra <gbatra at linux.ibm.com> writes:
> diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h
> index ce2b1b5eebdd..55a2ba36e9c4 100644
> --- a/arch/powerpc/include/asm/ppc-pci.h
> +++ b/arch/powerpc/include/asm/ppc-pci.h
> @@ -29,6 +29,9 @@ void *pci_traverse_device_nodes(struct device_node *start,
>  				void *(*fn)(struct device_node *, void *),
>  				void *data);
>  extern void pci_devs_phb_init_dynamic(struct pci_controller *phb);
> +extern void pci_register_device_dynamic(struct pci_controller *phb);
> +extern void pci_unregister_device_dynamic(struct pci_controller *phb);
> +
>  
>  /* From rtas_pci.h */
>  extern void init_pci_config_tokens (void);
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index ebe259bdd462..342739fe74c4 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1388,6 +1388,21 @@ static const struct attribute_group *spapr_tce_iommu_groups[] = {
>  	NULL,
>  };
>  
> +void pci_register_device_dynamic(struct pci_controller *phb)
> +{
> +	iommu_device_sysfs_add(&phb->iommu, phb->parent,
> +				spapr_tce_iommu_groups, "iommu-phb%04x",
> +				phb->global_number);
> +	iommu_device_register(&phb->iommu, &spapr_tce_iommu_ops,
> +				phb->parent);
> +}
> +
> +void pci_unregister_device_dynamic(struct pci_controller *phb)
> +{
> +	iommu_device_unregister(&phb->iommu);
> +	iommu_device_sysfs_remove(&phb->iommu);
> +}
> +
>  /*
>   * This registers IOMMU devices of PHBs. This needs to happen
>   * after core_initcall(iommu_init) + postcore_initcall(pci_driver_init) and
> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
> index 4ba824568119..ec70ca435b7e 100644
> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
> @@ -35,6 +35,8 @@ struct pci_controller *init_phb_dynamic(struct device_node *dn)
>  
>  	pseries_msi_allocate_domains(phb);
>  
> +	pci_register_device_dynamic(phb);
> +
>  	/* Create EEH devices for the PHB */
>  	eeh_phb_pe_create(phb);
>  
> @@ -76,6 +78,8 @@ int remove_phb_dynamic(struct pci_controller *phb)
>  		}
>  	}
>  
> +	pci_unregister_device_dynamic(phb);
> +
>  	pseries_msi_free_domains(phb);
>  
>  	/* Keep a reference so phb isn't freed yet */
> --

The change overall looks correct to me, but:

1. I don't think the new functions should use the "pci_" prefix; that
   should probably be reserved for code in the core PCI subsystem. Some
   existing code in arch/powerpc/kernel/iommu.c uses "ppc_iommu_" and
   "spapr_tce_", maybe one of those would work instead?

2. Your pci_register_device_dynamic() duplicates code from
   spapr_tce_setup_phb_iommus_initcall():

        list_for_each_entry(hose, &hose_list, list_node) {
                iommu_device_sysfs_add(&hose->iommu, hose->parent,
                                       spapr_tce_iommu_groups, "iommu-phb%04x",
                                       hose->global_number);
                iommu_device_register(&hose->iommu, &spapr_tce_iommu_ops,
                                      hose->parent);
        }

  Can the loop body be factored into a common function that can be
  used in both paths?