| Message ID | 20200212112514.2000-3-kishon@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Endpoint: Miscellaneous improvements | expand |
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 >
Hi Kishon, Did you get time to look into it? Thanks, Vidya Sagar On 6/11/2021 3:22 PM, Vidya Sagar wrote: > 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 >>
Hi Vidya Sagar, On 11/06/21 3:22 pm, Vidya Sagar wrote: > Hi Kishon, > Apologies for bringup it up this late. > I'm wondering if there was any issue which this patch tried to address? There was one function pci_epc_linkup() which was expected to be invoked in interrupt context (basically when the LINKUP interrupt is raised). But after it was moved to use atomic notifier, all the EPC core APIs were replaced to use mutex. > 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. I don't think it is ideal to initialize EPC in interrupt context (unless there is a specific reason for it). EPC initialization can be moved to bottom half similar to how commands are handled after LINKUP. Thanks Kishon > > 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 >>
On 6/21/2021 10:44 AM, Kishon Vijay Abraham I wrote: > External email: Use caution opening links or attachments > > > Hi Vidya Sagar, > > On 11/06/21 3:22 pm, Vidya Sagar wrote: >> Hi Kishon, >> Apologies for bringup it up this late. >> I'm wondering if there was any issue which this patch tried to address? > > There was one function pci_epc_linkup() which was expected to be invoked > in interrupt context (basically when the LINKUP interrupt is raised). > But after it was moved to use atomic notifier, all the EPC core APIs > were replaced to use mutex. >> 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. After reviewing the logs and code again, I think it was my mistake to come to early conclusion that it was because of calling mutex_lock() in the atomic context. It is clear now. I would like to understand the reason behind putting locks in the epc core driver before calling ops. I believe the ops callers should implement lock if they are concurrently accessing the ops instead of adding a global lock in the epc core. This would help in scenarios like the one below. We have a performance oriented endpoint function driver which calls map, unmap & raise_irq ops from softirq context and because of mutex_lock(), we can't do that now. epc core driver should not restrict the function drivers to use only non-atomic functions. Thanks, Vidya Sagar > > I don't think it is ideal to initialize EPC in interrupt context (unless > there is a specific reason for it). EPC initialization can be moved to > bottom half similar to how commands are handled after LINKUP. > > Thanks > Kishon > >> >> 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 >>>
Hi Vidya Sagar, On 21/06/21 3:08 pm, Vidya Sagar wrote: > > > On 6/21/2021 10:44 AM, Kishon Vijay Abraham I wrote: >> External email: Use caution opening links or attachments >> >> >> Hi Vidya Sagar, >> >> On 11/06/21 3:22 pm, Vidya Sagar wrote: >>> Hi Kishon, >>> Apologies for bringup it up this late. >>> I'm wondering if there was any issue which this patch tried to address? >> >> There was one function pci_epc_linkup() which was expected to be invoked >> in interrupt context (basically when the LINKUP interrupt is raised). >> But after it was moved to use atomic notifier, all the EPC core APIs >> were replaced to use mutex. >>> 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. > After reviewing the logs and code again, I think it was my mistake to > come to early conclusion that it was because of calling mutex_lock() in > the atomic context. It is clear now. > > I would like to understand the reason behind putting locks in the epc > core driver before calling ops. There could be two different functions trying to configure endpoint controller (could be a multi-function endpoint) and the framework should guarantee the hardware is not accessed by both the functions simultaneously. > I believe the ops callers should implement lock if they are concurrently > accessing the ops instead of adding a global lock in the epc core. This can only protect within a function and not across multiple functions. > This would help in scenarios like the one below. > > We have a performance oriented endpoint function driver which calls > map, unmap & raise_irq ops from softirq context and because of > mutex_lock(), we can't do that now. epc core driver should not restrict > the function drivers to use only non-atomic functions. Not sure what exactly the function driver does but can't map/unmap be done for a big block once to optimize and operate on that buffer? I'd assume you are having a custom driver on the host side too? Thanks Kishon > > Thanks, > Vidya Sagar >> >> I don't think it is ideal to initialize EPC in interrupt context (unless >> there is a specific reason for it). EPC initialization can be moved to >> bottom half similar to how commands are handled after LINKUP. > >> >> Thanks >> Kishon >> >>> >>> 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 >>>>
On 6/21/2021 7:07 PM, Kishon Vijay Abraham I wrote: > External email: Use caution opening links or attachments > > > Hi Vidya Sagar, > > On 21/06/21 3:08 pm, Vidya Sagar wrote: >> >> >> On 6/21/2021 10:44 AM, Kishon Vijay Abraham I wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> Hi Vidya Sagar, >>> >>> On 11/06/21 3:22 pm, Vidya Sagar wrote: >>>> Hi Kishon, >>>> Apologies for bringup it up this late. >>>> I'm wondering if there was any issue which this patch tried to address? >>> >>> There was one function pci_epc_linkup() which was expected to be invoked >>> in interrupt context (basically when the LINKUP interrupt is raised). >>> But after it was moved to use atomic notifier, all the EPC core APIs >>> were replaced to use mutex. >>>> 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. >> After reviewing the logs and code again, I think it was my mistake to >> come to early conclusion that it was because of calling mutex_lock() in >> the atomic context. It is clear now. >> >> I would like to understand the reason behind putting locks in the epc >> core driver before calling ops. > > There could be two different functions trying to configure endpoint > controller (could be a multi-function endpoint) and the framework should > guarantee the hardware is not accessed by both the functions simultaneously. Not all ops functions need to be protected by the lock. for ex:- get/set_msi(x)(), raise_irq(), get_features() don't need to be protected by a global lock. So, transferring the synchronization responsibility to the controller driver gives an efficient control on locking. > >> I believe the ops callers should implement lock if they are concurrently >> accessing the ops instead of adding a global lock in the epc core. > This can only protect within a function and not across multiple functions. >> This would help in scenarios like the one below. >> >> We have a performance oriented endpoint function driver which calls >> map, unmap & raise_irq ops from softirq context and because of >> mutex_lock(), we can't do that now. epc core driver should not restrict >> the function drivers to use only non-atomic functions. > > Not sure what exactly the function driver does but can't map/unmap be > done for a big block once to optimize and operate on that buffer? I'd > assume you are having a custom driver on the host side too? We implemented a function driver that provides a virtual ethernet interface. To complement this, we have a PCIe device driver on the host that exposes a virtual ethernet interface in the host system. start_xmit() in the function driver of the virtual ethernet interface is a soft irq which maps/unmaps each skb buffer dynamically. So, a one time static mapping is not possible. Similarly, because of the global lock, start_xmit() can not raise_irq() to the host. Thanks, Vidya Sagar > > Thanks > Kishon > >> >> Thanks, >> Vidya Sagar >>> >>> I don't think it is ideal to initialize EPC in interrupt context (unless >>> there is a specific reason for it). EPC initialization can be moved to >>> bottom half similar to how commands are handled after LINKUP. >> >>> >>> Thanks >>> Kishon >>> >>>> >>>> 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 >>>>>
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; };
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(-)