Le 10/09/2019 à 02:34, Alastair D'Silva a écrit :
On Mon, 2019-09-09 at 17:45 +0200, Frederic Barrat wrote:Protect the PHB's list of PE. Probably not needed as long as it was populated during PHB creation, but it feels right and will become required once we can add/remove opencapi devices on hotplug. Signed-off-by: Frederic Barrat <fbar...@linux.ibm.com> --- arch/powerpc/platforms/powernv/pci-ioda.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index 92767f006f20..3dbbf5365c1c 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -1080,8 +1080,9 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev) }/* Put PE to the list */+ mutex_lock(&phb->ioda.pe_list_mutex); list_add_tail(&pe->list, &phb->ioda.pe_list); - + mutex_unlock(&phb->ioda.pe_list_mutex); return pe; }@@ -3513,7 +3514,10 @@ static void pnv_ioda_release_pe(structpnv_ioda_pe *pe) struct pnv_phb *phb = pe->phb; struct pnv_ioda_pe *slave, *tmp;+ mutex_lock(&phb->ioda.pe_list_mutex);list_del(&pe->list); + mutex_unlock(&phb->ioda.pe_list_mutex); + switch (phb->type) { case PNV_PHB_IODA1: pnv_pci_ioda1_release_pe_dma(pe);Hmm, the ioda.pe_list_mutex muxtex exists, and is inited, but there are no other users. It's position & naming in the struct suggests it belongs to ioda.pe_list, rather than pnv_ioda_pe.list (as suggested by the lock/unlock around the list del).
I don't quite understand the above. The mutex is already in use by the functions handling virtual functions, pnv_ioda_setup_vf_PE() and pnv_ioda_release_vf_PE(). The point of the list is to keep track of all the PEs used by the PHB, so it makes sense to have the mutex at that level.
Do the other accessors of ioda.pe_list also need mutex protection? pnv_ioda_setup_bus_PE() pnv_pci_dma_bus_setup() pnv_pci_init_ioda_phb() pnv_pci_ioda_setup_PEs()
I think we could also use it there, it wouldn't hurt. Those functions are called when the kernel is building part of the PCI topology, and devices are not really active yet, so I don't think it's absolutely required.
I'm actually not sure my patch is needed either. With hotplug, the devices can come and go, whereas the PHB remains. So it feels right to start protecting the list when adding/removing a device. But I don't think we can really have concurrency and have 2 different operations adding/removing devices at the same time under the same PHB, at least for opencapi. Maybe for PCI, if we have multiple slots under the same PHB. Not sure.
Fred
If not, perhaps the metux should be removed from ioda and replaced with pe.list_mutex instead.
