diff mbox series

[v3,3/5] PCI: endpoint: Use a separate lock for protecting epc->pci_epf list

Message ID 20221006134927.41437-4-manivannan.sadhasivam@linaro.org (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: endpoint: Rework the EPC to EPF notification | expand

Commit Message

Manivannan Sadhasivam Oct. 6, 2022, 1:49 p.m. UTC
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>
---
 drivers/pci/endpoint/pci-epc-core.c | 9 +++++----
 include/linux/pci-epc.h             | 2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Kishon Vijay Abraham I Oct. 11, 2022, 12:40 p.m. UTC | #1
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;
>
diff mbox series

Patch

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;