lkml.org 
[lkml]   [2021]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
From
Date
Hi Kishon,
Apologies for bringup it up this late.
I'm wondering if there was any issue which this patch tried to address?
Actually, "The pci_epc_ops is not intended to be invoked from interrupt
context" isn't true in case of Tegra194. We do call
dw_pcie_ep_init_notify() API from threaded irq service routine and it
eventually calls mutext_lock() of pci_epc_get_features() which is
reusulting in the following warning log.
BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:
Would like hear your comments on it.

Thanks,
Vidya Sagar

On 2/12/2020 4:55 PM, Kishon Vijay Abraham I wrote:
> External email: Use caution opening links or attachments
>
>
> The pci_epc_ops is not intended to be invoked from interrupt context.
> Hence replace spin_lock_irqsave and spin_unlock_irqrestore with
> mutex_lock and mutex_unlock respectively.
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> drivers/pci/endpoint/pci-epc-core.c | 82 +++++++++++------------------
> include/linux/pci-epc.h | 6 +--
> 2 files changed, 34 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 2f6436599fcb..e51a12ed85bb 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -120,7 +120,6 @@ const struct pci_epc_features *pci_epc_get_features(struct pci_epc *epc,
> u8 func_no)
> {
> const struct pci_epc_features *epc_features;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> return NULL;
> @@ -128,9 +127,9 @@ const struct pci_epc_features *pci_epc_get_features(struct pci_epc *epc,
> if (!epc->ops->get_features)
> return NULL;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> epc_features = epc->ops->get_features(epc, func_no);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return epc_features;
> }
> @@ -144,14 +143,12 @@ EXPORT_SYMBOL_GPL(pci_epc_get_features);
> */
> void pci_epc_stop(struct pci_epc *epc)
> {
> - unsigned long flags;
> -
> if (IS_ERR(epc) || !epc->ops->stop)
> return;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> epc->ops->stop(epc);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
> }
> EXPORT_SYMBOL_GPL(pci_epc_stop);
>
> @@ -164,7 +161,6 @@ EXPORT_SYMBOL_GPL(pci_epc_stop);
> int pci_epc_start(struct pci_epc *epc)
> {
> int ret;
> - unsigned long flags;
>
> if (IS_ERR(epc))
> return -EINVAL;
> @@ -172,9 +168,9 @@ int pci_epc_start(struct pci_epc *epc)
> if (!epc->ops->start)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> ret = epc->ops->start(epc);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return ret;
> }
> @@ -193,7 +189,6 @@ int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no,
> enum pci_epc_irq_type type, u16 interrupt_num)
> {
> int ret;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> return -EINVAL;
> @@ -201,9 +196,9 @@ int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no,
> if (!epc->ops->raise_irq)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> ret = epc->ops->raise_irq(epc, func_no, type, interrupt_num);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return ret;
> }
> @@ -219,7 +214,6 @@ EXPORT_SYMBOL_GPL(pci_epc_raise_irq);
> int pci_epc_get_msi(struct pci_epc *epc, u8 func_no)
> {
> int interrupt;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> return 0;
> @@ -227,9 +221,9 @@ int pci_epc_get_msi(struct pci_epc *epc, u8 func_no)
> if (!epc->ops->get_msi)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> interrupt = epc->ops->get_msi(epc, func_no);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> if (interrupt < 0)
> return 0;
> @@ -252,7 +246,6 @@ int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 interrupts)
> {
> int ret;
> u8 encode_int;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> interrupts > 32)
> @@ -263,9 +256,9 @@ int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 interrupts)
>
> encode_int = order_base_2(interrupts);
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> ret = epc->ops->set_msi(epc, func_no, encode_int);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return ret;
> }
> @@ -281,7 +274,6 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msi);
> int pci_epc_get_msix(struct pci_epc *epc, u8 func_no)
> {
> int interrupt;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> return 0;
> @@ -289,9 +281,9 @@ int pci_epc_get_msix(struct pci_epc *epc, u8 func_no)
> if (!epc->ops->get_msix)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> interrupt = epc->ops->get_msix(epc, func_no);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> if (interrupt < 0)
> return 0;
> @@ -311,7 +303,6 @@ EXPORT_SYMBOL_GPL(pci_epc_get_msix);
> int pci_epc_set_msix(struct pci_epc *epc, u8 func_no, u16 interrupts)
> {
> int ret;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> interrupts < 1 || interrupts > 2048)
> @@ -320,9 +311,9 @@ int pci_epc_set_msix(struct pci_epc *epc, u8 func_no, u16 interrupts)
> if (!epc->ops->set_msix)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> ret = epc->ops->set_msix(epc, func_no, interrupts - 1);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return ret;
> }
> @@ -339,17 +330,15 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msix);
> void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no,
> phys_addr_t phys_addr)
> {
> - unsigned long flags;
> -
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> return;
>
> if (!epc->ops->unmap_addr)
> return;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> epc->ops->unmap_addr(epc, func_no, phys_addr);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
> }
> EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>
> @@ -367,7 +356,6 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
> phys_addr_t phys_addr, u64 pci_addr, size_t size)
> {
> int ret;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> return -EINVAL;
> @@ -375,9 +363,9 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
> if (!epc->ops->map_addr)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> ret = epc->ops->map_addr(epc, func_no, phys_addr, pci_addr, size);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return ret;
> }
> @@ -394,8 +382,6 @@ EXPORT_SYMBOL_GPL(pci_epc_map_addr);
> void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
> struct pci_epf_bar *epf_bar)
> {
> - unsigned long flags;
> -
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> (epf_bar->barno == BAR_5 &&
> epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
> @@ -404,9 +390,9 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
> if (!epc->ops->clear_bar)
> return;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> epc->ops->clear_bar(epc, func_no, epf_bar);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
> }
> EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
>
> @@ -422,7 +408,6 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> struct pci_epf_bar *epf_bar)
> {
> int ret;
> - unsigned long irq_flags;
> int flags = epf_bar->flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> @@ -437,9 +422,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> if (!epc->ops->set_bar)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, irq_flags);
> + mutex_lock(&epc->lock);
> ret = epc->ops->set_bar(epc, func_no, epf_bar);
> - spin_unlock_irqrestore(&epc->lock, irq_flags);
> + mutex_unlock(&epc->lock);
>
> return ret;
> }
> @@ -460,7 +445,6 @@ int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *header)
> {
> int ret;
> - unsigned long flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> return -EINVAL;
> @@ -468,9 +452,9 @@ int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
> if (!epc->ops->write_header)
> return 0;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> ret = epc->ops->write_header(epc, func_no, header);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return ret;
> }
> @@ -487,8 +471,6 @@ EXPORT_SYMBOL_GPL(pci_epc_write_header);
> */
> int pci_epc_add_epf(struct pci_epc *epc, struct pci_epf *epf)
> {
> - unsigned long flags;
> -
> if (epf->epc)
> return -EBUSY;
>
> @@ -500,9 +482,9 @@ int pci_epc_add_epf(struct pci_epc *epc, struct pci_epf *epf)
>
> epf->epc = epc;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> list_add_tail(&epf->list, &epc->pci_epf);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
>
> return 0;
> }
> @@ -517,15 +499,13 @@ EXPORT_SYMBOL_GPL(pci_epc_add_epf);
> */
> void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf)
> {
> - unsigned long flags;
> -
> if (!epc || IS_ERR(epc) || !epf)
> return;
>
> - spin_lock_irqsave(&epc->lock, flags);
> + mutex_lock(&epc->lock);
> list_del(&epf->list);
> epf->epc = NULL;
> - spin_unlock_irqrestore(&epc->lock, flags);
> + mutex_unlock(&epc->lock);
> }
> EXPORT_SYMBOL_GPL(pci_epc_remove_epf);
>
> @@ -604,7 +584,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
> goto err_ret;
> }
>
> - spin_lock_init(&epc->lock);
> + mutex_init(&epc->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 36644ccd32ac..9dd60f2e9705 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -88,7 +88,7 @@ struct pci_epc_mem {
> * @mem: address space of the endpoint controller
> * @max_functions: max number of functions that can be configured in this EPC
> * @group: configfs group representing the PCI EPC device
> - * @lock: spinlock to protect pci_epc ops
> + * @lock: mutex to protect pci_epc ops
> * @notifier: used to notify EPF of any EPC events (like linkup)
> */
> struct pci_epc {
> @@ -98,8 +98,8 @@ struct pci_epc {
> struct pci_epc_mem *mem;
> u8 max_functions;
> struct config_group *group;
> - /* spinlock to protect against concurrent access of EP controller */
> - spinlock_t lock;
> + /* mutex to protect against concurrent access of EP controller */
> + struct mutex lock;
> struct atomic_notifier_head notifier;
> };
>
> --
> 2.17.1
>

\
 
 \ /
  Last update: 2021-06-11 11:52    [W:0.110 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site