Re: [PATCH] powerpc/pseries/iommu: DLPAR ADD of pci device doesn't completely initialize pci_controller structure
From: Nathan Lynch <hidden>
Date: 2024-01-18 18:19:19
Hi Gaurav, A couple minor comments below. Gaurav Batra [off-list ref] writes:
quoted hunk ↗ jump to hunk
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) anddiff --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?