Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v3 3/5] PCI: endpoint: Use a separate lock for protecting
 epc->pci_epf list
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        <kishon@kernel.org>, <lpieralisi@kernel.org>, <bhelgaas@google.com>
CC:     <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kw@linux.com>, <robh@kernel.org>, <vidyas@nvidia.com>,
        <vigneshr@ti.com>
References: <20221006134927.41437-1-manivannan.sadhasivam@linaro.org>
 <20221006134927.41437-4-manivannan.sadhasivam@linaro.org>
From:   Kishon Vijay Abraham I <kishon@ti.com>
Message-ID: <5d041111-6ebc-4ac6-5693-443535545510@ti.com>
Date:   Tue, 11 Oct 2022 18:10:08 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20221006134927.41437-4-manivannan.sadhasivam@linaro.org>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 06/10/22 7:19 pm, Manivannan Sadhasivam wrote:
> The EPC controller maintains a list of EPF drivers added to it. For
> protecting this list against the concurrent accesses, the epc->lock
> (used for protecting epc_ops) has been used so far. Since there were
> no users trying to use epc_ops and modify the pci_epf list simultaneously,
> this was not an issue.
> 
> But with the addition of callback mechanism for passing the events, this
> will be a problem. Because the pci_epf list needs to be iterated first
> for getting hold of the EPF driver and then the relevant event specific
> callback needs to be called for the driver.
> 
> If the same epc->lock is used, then it will result in a deadlock scenario.
> 
> For instance,
> 
> ...
> 	mutex_lock(&epc->lock);
> 	list_for_each_entry(epf, &epc->pci_epf, list) {
> 		epf->event_ops->core_init(epf);
> 		|
> 		|-> pci_epc_set_bar();
> 			|
> 			|-> mutex_lock(&epc->lock) # DEADLOCK
> ...
> 
> So to fix this issue, use a separate lock called "list_lock" for
> protecting the pci_epf list against the concurrent accesses. This lock
> will also be used by the callback mechanism.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>   drivers/pci/endpoint/pci-epc-core.c | 9 +++++----
>   include/linux/pci-epc.h             | 2 ++
>   2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 3bc9273d0a08..6cce430d431b 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -613,7 +613,7 @@ int pci_epc_add_epf(struct pci_epc *epc, struct pci_epf *epf,
>   	if (type == SECONDARY_INTERFACE && epf->sec_epc)
>   		return -EBUSY;
>   
> -	mutex_lock(&epc->lock);
> +	mutex_lock(&epc->list_lock);
>   	func_no = find_first_zero_bit(&epc->function_num_map,
>   				      BITS_PER_LONG);
>   	if (func_no >= BITS_PER_LONG) {
> @@ -640,7 +640,7 @@ int pci_epc_add_epf(struct pci_epc *epc, struct pci_epf *epf,
>   
>   	list_add_tail(list, &epc->pci_epf);
>   ret:
> -	mutex_unlock(&epc->lock);
> +	mutex_unlock(&epc->list_lock);
>   
>   	return ret;
>   }
> @@ -672,11 +672,11 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf,
>   		list = &epf->sec_epc_list;
>   	}
>   
> -	mutex_lock(&epc->lock);
> +	mutex_lock(&epc->list_lock);
>   	clear_bit(func_no, &epc->function_num_map);
>   	list_del(list);
>   	epf->epc = NULL;
> -	mutex_unlock(&epc->lock);
> +	mutex_unlock(&epc->list_lock);
>   }
>   EXPORT_SYMBOL_GPL(pci_epc_remove_epf);
>   
> @@ -773,6 +773,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
>   	}
>   
>   	mutex_init(&epc->lock);
> +	mutex_init(&epc->list_lock);
>   	INIT_LIST_HEAD(&epc->pci_epf);
>   	ATOMIC_INIT_NOTIFIER_HEAD(&epc->notifier);
>   
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index a48778e1a4ee..fe729dfe509b 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -122,6 +122,7 @@ struct pci_epc_mem {
>    * struct pci_epc - represents the PCI EPC device
>    * @dev: PCI EPC device
>    * @pci_epf: list of endpoint functions present in this EPC device
> + * list_lock: Mutex for protecting pci_epf list
>    * @ops: function pointers for performing endpoint operations
>    * @windows: array of address space of the endpoint controller
>    * @mem: first window of the endpoint controller, which corresponds to
> @@ -139,6 +140,7 @@ struct pci_epc_mem {
>   struct pci_epc {
>   	struct device			dev;
>   	struct list_head		pci_epf;
> +	struct mutex			list_lock;
>   	const struct pci_epc_ops	*ops;
>   	struct pci_epc_mem		**windows;
>   	struct pci_epc_mem		*mem;
>