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

[Kishon Vijay Abraham I]

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@xxxxxxxxxx>

Acked-by: Kishon Vijay Abraham I <kishon@xxxxxx>
> ---
>   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;