Re: [PATCH 02/11] powerpc/powernv/ioda: Protect PE list

[Frederic Barrat <fbarrat@linux.ibm.com>]

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 <fbarrat@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(struct
>> pnv_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.
>