LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Frederic Barrat <fbarrat@linux.ibm.com>
To: "Alastair D'Silva" <alastair@au1.ibm.com>,
	linuxppc-dev@lists.ozlabs.org,  andrew.donnellan@au1.ibm.com,
	clombard@linux.ibm.com
Cc: groug@kaod.org
Subject: Re: [PATCH 02/11] powerpc/powernv/ioda: Protect PE list
Date: Tue, 19 Nov 2019 13:55:27 +0100
Message-ID: <882c0d26-7931-a2e9-c99a-7732d32a6a2f@linux.ibm.com> (raw)
In-Reply-To: <8f5d581d8f1e8defaf8622cd79c40c98f18d3507.camel@au1.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.
> 


  reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-09 15:45 [PATCH 00/11] opencapi: enable card reset and link retraining Frederic Barrat
2019-09-09 15:45 ` [PATCH 01/11] powerpc/powernv/ioda: Fix ref count for devices with their own PE Frederic Barrat
2019-09-10  0:20   ` Alastair D'Silva
2019-09-26 16:44   ` Andrew Donnellan
2019-09-26 17:15     ` Frederic Barrat
2019-09-27  6:54       ` Alexey Kardashevskiy
2019-11-12 17:38         ` Frederic Barrat
2019-11-18  1:04           ` Alistair Popple
2019-11-18  1:24             ` Oliver O'Halloran
2019-11-18  2:36               ` Alistair Popple
2019-11-18 18:21                 ` Frederic Barrat
2019-09-09 15:45 ` [PATCH 02/11] powerpc/powernv/ioda: Protect PE list Frederic Barrat
2019-09-10  0:34   ` Alastair D'Silva
2019-11-19 12:55     ` Frederic Barrat [this message]
2019-11-19 13:22       ` Oliver O'Halloran
2019-11-19 14:36         ` Frederic Barrat
2019-11-19  5:55   ` Andrew Donnellan
2019-09-09 15:45 ` [PATCH 03/11] powerpc/powernv/ioda: set up PE on opencapi device when enabling Frederic Barrat
2019-09-10  0:38   ` Alastair D'Silva
2019-09-27 16:43   ` Andrew Donnellan
2019-09-09 15:45 ` [PATCH 04/11] powerpc/powernv/ioda: Release opencapi device Frederic Barrat
2019-09-10  0:56   ` Alastair D'Silva
2019-11-19 17:32     ` Frederic Barrat
2019-11-19  7:08   ` Andrew Donnellan
2019-09-09 15:45 ` [PATCH 05/11] powerpc/powernv/ioda: Find opencapi slot for a device node Frederic Barrat
2019-09-10  0:57   ` Alastair D'Silva
2019-11-19  1:26   ` Andrew Donnellan
2019-11-19 14:33     ` Frederic Barrat
2019-09-09 15:45 ` [PATCH 06/11] pci/hotplug/pnv-php: Remove erroneous warning Frederic Barrat
2019-09-10  0:58   ` Alastair D'Silva
2019-11-19  5:00   ` Andrew Donnellan
2019-09-09 15:45 ` [PATCH 07/11] pci/hotplug/pnv-php: Improve error msg on power state change failure Frederic Barrat
2019-09-10  0:59   ` Alastair D'Silva
2019-11-19  2:23   ` Andrew Donnellan
2019-09-09 15:45 ` [PATCH 08/11] pci/hotplug/pnv-php: Register opencapi slots Frederic Barrat
2019-09-10  1:00   ` Alastair D'Silva
2019-11-19  5:18   ` Andrew Donnellan
2019-11-19 15:15     ` Frederic Barrat
2019-09-09 15:45 ` [PATCH 09/11] pci/hotplug/pnv-php: Relax check when disabling slot Frederic Barrat
2019-09-10  1:00   ` Alastair D'Silva
2019-09-27 15:56   ` Andrew Donnellan
2019-09-09 15:45 ` [PATCH 10/11] pci/hotplug/pnv-php: Wrap warnings in macro Frederic Barrat
2019-09-10  1:03   ` Alastair D'Silva
2019-09-26 17:18   ` Andrew Donnellan
2019-09-09 15:46 ` [PATCH 11/11] ocxl: Add PCI hotplug dependency to Kconfig Frederic Barrat
2019-09-10  1:03   ` Alastair D'Silva
2019-09-26 17:11   ` Andrew Donnellan
2019-09-24  4:24 ` [PATCH 00/11] opencapi: enable card reset and link retraining Alexey Kardashevskiy
2019-09-24  6:39   ` Frederic Barrat
2019-09-25  0:20     ` Alexey Kardashevskiy

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=882c0d26-7931-a2e9-c99a-7732d32a6a2f@linux.ibm.com \
    --to=fbarrat@linux.ibm.com \
    --cc=alastair@au1.ibm.com \
    --cc=andrew.donnellan@au1.ibm.com \
    --cc=clombard@linux.ibm.com \
    --cc=groug@kaod.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git