* [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements
@ 2020-02-12 11:25 Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 1/5] PCI: endpoint: Use notification chain mechanism to notify EPC events to EPF Kishon Vijay Abraham I
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2020-02-12 11:25 UTC (permalink / raw)
To: Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Vidya Sagar,
Athani Nadeem Ladkhan, Tom Joseph
Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I
Changes from v1:
Rebased to Linux 5.6-rc1 and removed dependencies to my other series
to unblock [1]
[1] -> http://lore.kernel.org/r/20200103100736.27627-1-vidyas@nvidia.com
v1 of this patch series can be found @
http://lore.kernel.org/r/20191231100331.6316-1-kishon@ti.com
This series adds miscellaneous improvements to PCIe endpoint core.
1) Protect concurrent access to memory allocation in pci-epc-mem
2) Replace spinlock with mutex in pci-epc-core and also use
notification chain mechanism to notify EPC events to EPF driver.
3) Since endpoint function device can be created by multiple
mechanisms (configfs, devicetree, etc..), allowing each of these
mechanisms to assign a function number would result in mutliple
endpoint function devices having the same function number. In order
to avoid this, let EPC core assign a function number to the
endpoint device.
Kishon Vijay Abraham I (5):
PCI: endpoint: Use notification chain mechanism to notify EPC events
to EPF
PCI: endpoint: Replace spinlock with mutex
PCI: endpoint: Protect concurrent access to memory allocation with
mutex
PCI: endpoint: Protect concurrent access to pci_epf_ops with mutex
PCI: endpoint: Assign function number for each PF in EPC core
drivers/pci/endpoint/functions/pci-epf-test.c | 13 +-
drivers/pci/endpoint/pci-ep-cfs.c | 27 +----
drivers/pci/endpoint/pci-epc-core.c | 113 ++++++++----------
drivers/pci/endpoint/pci-epc-mem.c | 10 +-
drivers/pci/endpoint/pci-epf-core.c | 33 ++---
include/linux/pci-epc.h | 19 ++-
include/linux/pci-epf.h | 9 +-
7 files changed, 108 insertions(+), 116 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/5] PCI: endpoint: Use notification chain mechanism to notify EPC events to EPF
2020-02-12 11:25 [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Kishon Vijay Abraham I
@ 2020-02-12 11:25 ` Kishon Vijay Abraham I
2020-02-17 12:14 ` Vidya Sagar
2020-02-12 11:25 ` [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex Kishon Vijay Abraham I
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2020-02-12 11:25 UTC (permalink / raw)
To: Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Vidya Sagar,
Athani Nadeem Ladkhan, Tom Joseph
Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I
Use atomic_notifier_call_chain() to notify EPC events like linkup to EPF
driver instead of using linkup ops in EPF driver. This is in preparation
for adding proper locking mechanism to EPF ops. This will also enable to
add more events (in addition to linkup) in the future.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
drivers/pci/endpoint/functions/pci-epf-test.c | 13 ++++++++---
drivers/pci/endpoint/pci-epc-core.c | 9 ++------
drivers/pci/endpoint/pci-epf-core.c | 22 +------------------
include/linux/pci-epc.h | 8 +++++++
include/linux/pci-epf.h | 6 ++---
5 files changed, 23 insertions(+), 35 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 5d74f81ddfe4..bddff15052cc 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -360,12 +360,16 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
msecs_to_jiffies(1));
}
-static void pci_epf_test_linkup(struct pci_epf *epf)
+static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
+ void *data)
{
+ struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
struct pci_epf_test *epf_test = epf_get_drvdata(epf);
queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
msecs_to_jiffies(1));
+
+ return NOTIFY_OK;
}
static void pci_epf_test_unbind(struct pci_epf *epf)
@@ -546,8 +550,12 @@ static int pci_epf_test_bind(struct pci_epf *epf)
}
}
- if (!linkup_notifier)
+ if (linkup_notifier) {
+ epf->nb.notifier_call = pci_epf_test_notifier;
+ pci_epc_register_notifier(epc, &epf->nb);
+ } else {
queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
+ }
return 0;
}
@@ -580,7 +588,6 @@ static int pci_epf_test_probe(struct pci_epf *epf)
static struct pci_epf_ops ops = {
.unbind = pci_epf_test_unbind,
.bind = pci_epf_test_bind,
- .linkup = pci_epf_test_linkup,
};
static struct pci_epf_driver test_driver = {
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 2091508c1620..2f6436599fcb 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -539,16 +539,10 @@ EXPORT_SYMBOL_GPL(pci_epc_remove_epf);
*/
void pci_epc_linkup(struct pci_epc *epc)
{
- unsigned long flags;
- struct pci_epf *epf;
-
if (!epc || IS_ERR(epc))
return;
- spin_lock_irqsave(&epc->lock, flags);
- list_for_each_entry(epf, &epc->pci_epf, list)
- pci_epf_linkup(epf);
- spin_unlock_irqrestore(&epc->lock, flags);
+ atomic_notifier_call_chain(&epc->notifier, 0, NULL);
}
EXPORT_SYMBOL_GPL(pci_epc_linkup);
@@ -612,6 +606,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
spin_lock_init(&epc->lock);
INIT_LIST_HEAD(&epc->pci_epf);
+ ATOMIC_INIT_NOTIFIER_HEAD(&epc->notifier);
device_initialize(&epc->dev);
epc->dev.class = pci_epc_class;
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index fb1306de8f40..93f28c65ace0 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -20,26 +20,6 @@ static DEFINE_MUTEX(pci_epf_mutex);
static struct bus_type pci_epf_bus_type;
static const struct device_type pci_epf_type;
-/**
- * pci_epf_linkup() - Notify the function driver that EPC device has
- * established a connection with the Root Complex.
- * @epf: the EPF device bound to the EPC device which has established
- * the connection with the host
- *
- * Invoke to notify the function driver that EPC device has established
- * a connection with the Root Complex.
- */
-void pci_epf_linkup(struct pci_epf *epf)
-{
- if (!epf->driver) {
- dev_WARN(&epf->dev, "epf device not bound to driver\n");
- return;
- }
-
- epf->driver->ops->linkup(epf);
-}
-EXPORT_SYMBOL_GPL(pci_epf_linkup);
-
/**
* pci_epf_unbind() - Notify the function driver that the binding between the
* EPF device and EPC device has been lost
@@ -214,7 +194,7 @@ int __pci_epf_register_driver(struct pci_epf_driver *driver,
if (!driver->ops)
return -EINVAL;
- if (!driver->ops->bind || !driver->ops->unbind || !driver->ops->linkup)
+ if (!driver->ops->bind || !driver->ops->unbind)
return -EINVAL;
driver->driver.bus = &pci_epf_bus_type;
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 56f1846b9d39..36644ccd32ac 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -89,6 +89,7 @@ struct pci_epc_mem {
* @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
+ * @notifier: used to notify EPF of any EPC events (like linkup)
*/
struct pci_epc {
struct device dev;
@@ -99,6 +100,7 @@ struct pci_epc {
struct config_group *group;
/* spinlock to protect against concurrent access of EP controller */
spinlock_t lock;
+ struct atomic_notifier_head notifier;
};
/**
@@ -141,6 +143,12 @@ static inline void *epc_get_drvdata(struct pci_epc *epc)
return dev_get_drvdata(&epc->dev);
}
+static inline int
+pci_epc_register_notifier(struct pci_epc *epc, struct notifier_block *nb)
+{
+ return atomic_notifier_chain_register(&epc->notifier, nb);
+}
+
struct pci_epc *
__devm_pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
struct module *owner);
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 2d6f07556682..4993f7f6439b 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -55,13 +55,10 @@ struct pci_epf_header {
* @bind: ops to perform when a EPC device has been bound to EPF device
* @unbind: ops to perform when a binding has been lost between a EPC device
* and EPF device
- * @linkup: ops to perform when the EPC device has established a connection with
- * a host system
*/
struct pci_epf_ops {
int (*bind)(struct pci_epf *epf);
void (*unbind)(struct pci_epf *epf);
- void (*linkup)(struct pci_epf *epf);
};
/**
@@ -112,6 +109,7 @@ struct pci_epf_bar {
* @epc: the EPC device to which this EPF device is bound
* @driver: the EPF driver to which this EPF device is bound
* @list: to add pci_epf as a list of PCI endpoint functions to pci_epc
+ * @nb: notifier block to notify EPF of any EPC events (like linkup)
*/
struct pci_epf {
struct device dev;
@@ -125,6 +123,7 @@ struct pci_epf {
struct pci_epc *epc;
struct pci_epf_driver *driver;
struct list_head list;
+ struct notifier_block nb;
};
#define to_pci_epf(epf_dev) container_of((epf_dev), struct pci_epf, dev)
@@ -154,5 +153,4 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar);
int pci_epf_bind(struct pci_epf *epf);
void pci_epf_unbind(struct pci_epf *epf);
-void pci_epf_linkup(struct pci_epf *epf);
#endif /* __LINUX_PCI_EPF_H */
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
2020-02-12 11:25 [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 1/5] PCI: endpoint: Use notification chain mechanism to notify EPC events to EPF Kishon Vijay Abraham I
@ 2020-02-12 11:25 ` Kishon Vijay Abraham I
2021-06-11 9:52 ` Vidya Sagar
2020-02-12 11:25 ` [PATCH v2 3/5] PCI: endpoint: Protect concurrent access to memory allocation " Kishon Vijay Abraham I
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2020-02-12 11:25 UTC (permalink / raw)
To: Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Vidya Sagar,
Athani Nadeem Ladkhan, Tom Joseph
Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] PCI: endpoint: Protect concurrent access to memory allocation with mutex
2020-02-12 11:25 [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 1/5] PCI: endpoint: Use notification chain mechanism to notify EPC events to EPF Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex Kishon Vijay Abraham I
@ 2020-02-12 11:25 ` Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 4/5] PCI: endpoint: Protect concurrent access to pci_epf_ops " Kishon Vijay Abraham I
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2020-02-12 11:25 UTC (permalink / raw)
To: Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Vidya Sagar,
Athani Nadeem Ladkhan, Tom Joseph
Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I
pci-epc-mem uses bitmap to manage the PCI address region. This address
region will be shared by multiple endpoint functions (in the case of
multi function endpoint) and it has to be protected from concurrent
access to avoid updating inconsistent state. Use mutex to protect while
updating bitmap.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
drivers/pci/endpoint/pci-epc-mem.c | 10 ++++++++--
include/linux/pci-epc.h | 3 +++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
index d2b174ce15de..abfac1109a13 100644
--- a/drivers/pci/endpoint/pci-epc-mem.c
+++ b/drivers/pci/endpoint/pci-epc-mem.c
@@ -79,6 +79,7 @@ int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_base, size_t size,
mem->page_size = page_size;
mem->pages = pages;
mem->size = size;
+ mutex_init(&mem->lock);
epc->mem = mem;
@@ -122,7 +123,7 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
phys_addr_t *phys_addr, size_t size)
{
int pageno;
- void __iomem *virt_addr;
+ void __iomem *virt_addr = NULL;
struct pci_epc_mem *mem = epc->mem;
unsigned int page_shift = ilog2(mem->page_size);
int order;
@@ -130,15 +131,18 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
size = ALIGN(size, mem->page_size);
order = pci_epc_mem_get_order(mem, size);
+ mutex_lock(&mem->lock);
pageno = bitmap_find_free_region(mem->bitmap, mem->pages, order);
if (pageno < 0)
- return NULL;
+ goto ret;
*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
virt_addr = ioremap(*phys_addr, size);
if (!virt_addr)
bitmap_release_region(mem->bitmap, pageno, order);
+ret:
+ mutex_unlock(&mem->lock);
return virt_addr;
}
EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
@@ -164,7 +168,9 @@ void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
pageno = (phys_addr - mem->phys_base) >> page_shift;
size = ALIGN(size, mem->page_size);
order = pci_epc_mem_get_order(mem, size);
+ mutex_lock(&mem->lock);
bitmap_release_region(mem->bitmap, pageno, order);
+ mutex_unlock(&mem->lock);
}
EXPORT_SYMBOL_GPL(pci_epc_mem_free_addr);
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 9dd60f2e9705..4e3e527c49d1 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -71,6 +71,7 @@ struct pci_epc_ops {
* @bitmap: bitmap to manage the PCI address space
* @pages: number of bits representing the address region
* @page_size: size of each page
+ * @lock: mutex to protect bitmap
*/
struct pci_epc_mem {
phys_addr_t phys_base;
@@ -78,6 +79,8 @@ struct pci_epc_mem {
unsigned long *bitmap;
size_t page_size;
int pages;
+ /* mutex to protect against concurrent access for memory allocation*/
+ struct mutex lock;
};
/**
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] PCI: endpoint: Protect concurrent access to pci_epf_ops with mutex
2020-02-12 11:25 [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Kishon Vijay Abraham I
` (2 preceding siblings ...)
2020-02-12 11:25 ` [PATCH v2 3/5] PCI: endpoint: Protect concurrent access to memory allocation " Kishon Vijay Abraham I
@ 2020-02-12 11:25 ` Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 5/5] PCI: endpoint: Assign function number for each PF in EPC core Kishon Vijay Abraham I
2020-02-21 15:54 ` [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Lorenzo Pieralisi
5 siblings, 0 replies; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2020-02-12 11:25 UTC (permalink / raw)
To: Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Vidya Sagar,
Athani Nadeem Ladkhan, Tom Joseph
Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I
Protect concurrent access to pci_epf_ops with mutex.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
drivers/pci/endpoint/pci-epf-core.c | 11 ++++++++++-
include/linux/pci-epf.h | 3 +++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 93f28c65ace0..6e0648991b5c 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -35,7 +35,9 @@ void pci_epf_unbind(struct pci_epf *epf)
return;
}
+ mutex_lock(&epf->lock);
epf->driver->ops->unbind(epf);
+ mutex_unlock(&epf->lock);
module_put(epf->driver->owner);
}
EXPORT_SYMBOL_GPL(pci_epf_unbind);
@@ -49,6 +51,8 @@ EXPORT_SYMBOL_GPL(pci_epf_unbind);
*/
int pci_epf_bind(struct pci_epf *epf)
{
+ int ret;
+
if (!epf->driver) {
dev_WARN(&epf->dev, "epf device not bound to driver\n");
return -EINVAL;
@@ -57,7 +61,11 @@ int pci_epf_bind(struct pci_epf *epf)
if (!try_module_get(epf->driver->owner))
return -EAGAIN;
- return epf->driver->ops->bind(epf);
+ mutex_lock(&epf->lock);
+ ret = epf->driver->ops->bind(epf);
+ mutex_unlock(&epf->lock);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(pci_epf_bind);
@@ -252,6 +260,7 @@ struct pci_epf *pci_epf_create(const char *name)
device_initialize(dev);
dev->bus = &pci_epf_bus_type;
dev->type = &pci_epf_type;
+ mutex_init(&epf->lock);
ret = dev_set_name(dev, "%s", name);
if (ret) {
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 4993f7f6439b..bcdf4f07bde7 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -110,6 +110,7 @@ struct pci_epf_bar {
* @driver: the EPF driver to which this EPF device is bound
* @list: to add pci_epf as a list of PCI endpoint functions to pci_epc
* @nb: notifier block to notify EPF of any EPC events (like linkup)
+ * @lock: mutex to protect pci_epf_ops
*/
struct pci_epf {
struct device dev;
@@ -124,6 +125,8 @@ struct pci_epf {
struct pci_epf_driver *driver;
struct list_head list;
struct notifier_block nb;
+ /* mutex to protect against concurrent access of pci_epf_ops */
+ struct mutex lock;
};
#define to_pci_epf(epf_dev) container_of((epf_dev), struct pci_epf, dev)
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 5/5] PCI: endpoint: Assign function number for each PF in EPC core
2020-02-12 11:25 [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Kishon Vijay Abraham I
` (3 preceding siblings ...)
2020-02-12 11:25 ` [PATCH v2 4/5] PCI: endpoint: Protect concurrent access to pci_epf_ops " Kishon Vijay Abraham I
@ 2020-02-12 11:25 ` Kishon Vijay Abraham I
2020-02-21 15:54 ` [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Lorenzo Pieralisi
5 siblings, 0 replies; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2020-02-12 11:25 UTC (permalink / raw)
To: Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Vidya Sagar,
Athani Nadeem Ladkhan, Tom Joseph
Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I
The PCIe endpoint core relies on the drivers that invoke the
pci_epc_add_epf() API to allocate and assign a function number
to each physical function (PF). Since endpoint function device can
be created by multiple mechanisms (configfs, devicetree, etc..),
allowing each of these mechanisms to assign a function number
would result in mutliple endpoint function devices having the
same function number. In order to avoid this, let EPC core assign
a function number to the endpoint device.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
drivers/pci/endpoint/pci-ep-cfs.c | 27 +++++----------------------
drivers/pci/endpoint/pci-epc-core.c | 26 ++++++++++++++++++++++----
include/linux/pci-epc.h | 2 ++
3 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index d1288a0bd530..e7e8367eead1 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -29,7 +29,6 @@ struct pci_epc_group {
struct config_group group;
struct pci_epc *epc;
bool start;
- unsigned long function_num_map;
};
static inline struct pci_epf_group *to_pci_epf_group(struct config_item *item)
@@ -89,37 +88,22 @@ static int pci_epc_epf_link(struct config_item *epc_item,
struct config_item *epf_item)
{
int ret;
- u32 func_no = 0;
struct pci_epf_group *epf_group = to_pci_epf_group(epf_item);
struct pci_epc_group *epc_group = to_pci_epc_group(epc_item);
struct pci_epc *epc = epc_group->epc;
struct pci_epf *epf = epf_group->epf;
- func_no = find_first_zero_bit(&epc_group->function_num_map,
- BITS_PER_LONG);
- if (func_no >= BITS_PER_LONG)
- return -EINVAL;
-
- set_bit(func_no, &epc_group->function_num_map);
- epf->func_no = func_no;
-
ret = pci_epc_add_epf(epc, epf);
if (ret)
- goto err_add_epf;
+ return ret;
ret = pci_epf_bind(epf);
- if (ret)
- goto err_epf_bind;
+ if (ret) {
+ pci_epc_remove_epf(epc, epf);
+ return ret;
+ }
return 0;
-
-err_epf_bind:
- pci_epc_remove_epf(epc, epf);
-
-err_add_epf:
- clear_bit(func_no, &epc_group->function_num_map);
-
- return ret;
}
static void pci_epc_epf_unlink(struct config_item *epc_item,
@@ -134,7 +118,6 @@ static void pci_epc_epf_unlink(struct config_item *epc_item,
epc = epc_group->epc;
epf = epf_group->epf;
- clear_bit(epf->func_no, &epc_group->function_num_map);
pci_epf_unbind(epf);
pci_epc_remove_epf(epc, epf);
}
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index e51a12ed85bb..dc1c673534e0 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -471,22 +471,39 @@ EXPORT_SYMBOL_GPL(pci_epc_write_header);
*/
int pci_epc_add_epf(struct pci_epc *epc, struct pci_epf *epf)
{
+ u32 func_no;
+ int ret = 0;
+
if (epf->epc)
return -EBUSY;
if (IS_ERR(epc))
return -EINVAL;
- if (epf->func_no > epc->max_functions - 1)
- return -EINVAL;
+ mutex_lock(&epc->lock);
+ func_no = find_first_zero_bit(&epc->function_num_map,
+ BITS_PER_LONG);
+ if (func_no >= BITS_PER_LONG) {
+ ret = -EINVAL;
+ goto ret;
+ }
+
+ if (func_no > epc->max_functions - 1) {
+ dev_err(&epc->dev, "Exceeding max supported Function Number\n");
+ ret = -EINVAL;
+ goto ret;
+ }
+ set_bit(func_no, &epc->function_num_map);
+ epf->func_no = func_no;
epf->epc = epc;
- mutex_lock(&epc->lock);
list_add_tail(&epf->list, &epc->pci_epf);
+
+ret:
mutex_unlock(&epc->lock);
- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(pci_epc_add_epf);
@@ -503,6 +520,7 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf)
return;
mutex_lock(&epc->lock);
+ clear_bit(epf->func_no, &epc->function_num_map);
list_del(&epf->list);
epf->epc = NULL;
mutex_unlock(&epc->lock);
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 4e3e527c49d1..ccaf6e3fa931 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -92,6 +92,7 @@ struct pci_epc_mem {
* @max_functions: max number of functions that can be configured in this EPC
* @group: configfs group representing the PCI EPC device
* @lock: mutex to protect pci_epc ops
+ * @function_num_map: bitmap to manage physical function number
* @notifier: used to notify EPF of any EPC events (like linkup)
*/
struct pci_epc {
@@ -103,6 +104,7 @@ struct pci_epc {
struct config_group *group;
/* mutex to protect against concurrent access of EP controller */
struct mutex lock;
+ unsigned long function_num_map;
struct atomic_notifier_head notifier;
};
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/5] PCI: endpoint: Use notification chain mechanism to notify EPC events to EPF
2020-02-12 11:25 ` [PATCH v2 1/5] PCI: endpoint: Use notification chain mechanism to notify EPC events to EPF Kishon Vijay Abraham I
@ 2020-02-17 12:14 ` Vidya Sagar
0 siblings, 0 replies; 14+ messages in thread
From: Vidya Sagar @ 2020-02-17 12:14 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Lorenzo Pieralisi, Bjorn Helgaas,
Andrew Murray, Athani Nadeem Ladkhan, Tom Joseph
Cc: linux-pci, linux-kernel
On 2/12/2020 4:55 PM, Kishon Vijay Abraham I wrote:
> External email: Use caution opening links or attachments
>
>
> Use atomic_notifier_call_chain() to notify EPC events like linkup to EPF
> driver instead of using linkup ops in EPF driver. This is in preparation
> for adding proper locking mechanism to EPF ops. This will also enable to
> add more events (in addition to linkup) in the future.
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 13 ++++++++---
> drivers/pci/endpoint/pci-epc-core.c | 9 ++------
> drivers/pci/endpoint/pci-epf-core.c | 22 +------------------
> include/linux/pci-epc.h | 8 +++++++
> include/linux/pci-epf.h | 6 ++---
> 5 files changed, 23 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 5d74f81ddfe4..bddff15052cc 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -360,12 +360,16 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
> msecs_to_jiffies(1));
> }
>
> -static void pci_epf_test_linkup(struct pci_epf *epf)
> +static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
> + void *data)
> {
> + struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
> struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>
> queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
> msecs_to_jiffies(1));
> +
> + return NOTIFY_OK;
> }
>
> static void pci_epf_test_unbind(struct pci_epf *epf)
> @@ -546,8 +550,12 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> }
> }
>
> - if (!linkup_notifier)
> + if (linkup_notifier) {
> + epf->nb.notifier_call = pci_epf_test_notifier;
> + pci_epc_register_notifier(epc, &epf->nb);
> + } else {
> queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
> + }
>
> return 0;
> }
> @@ -580,7 +588,6 @@ static int pci_epf_test_probe(struct pci_epf *epf)
> static struct pci_epf_ops ops = {
> .unbind = pci_epf_test_unbind,
> .bind = pci_epf_test_bind,
> - .linkup = pci_epf_test_linkup,
> };
>
> static struct pci_epf_driver test_driver = {
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 2091508c1620..2f6436599fcb 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -539,16 +539,10 @@ EXPORT_SYMBOL_GPL(pci_epc_remove_epf);
> */
> void pci_epc_linkup(struct pci_epc *epc)
> {
> - unsigned long flags;
> - struct pci_epf *epf;
> -
> if (!epc || IS_ERR(epc))
> return;
>
> - spin_lock_irqsave(&epc->lock, flags);
> - list_for_each_entry(epf, &epc->pci_epf, list)
> - pci_epf_linkup(epf);
> - spin_unlock_irqrestore(&epc->lock, flags);
> + atomic_notifier_call_chain(&epc->notifier, 0, NULL);
> }
> EXPORT_SYMBOL_GPL(pci_epc_linkup);
>
> @@ -612,6 +606,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
>
> spin_lock_init(&epc->lock);
> INIT_LIST_HEAD(&epc->pci_epf);
> + ATOMIC_INIT_NOTIFIER_HEAD(&epc->notifier);
>
> device_initialize(&epc->dev);
> epc->dev.class = pci_epc_class;
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index fb1306de8f40..93f28c65ace0 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -20,26 +20,6 @@ static DEFINE_MUTEX(pci_epf_mutex);
> static struct bus_type pci_epf_bus_type;
> static const struct device_type pci_epf_type;
>
> -/**
> - * pci_epf_linkup() - Notify the function driver that EPC device has
> - * established a connection with the Root Complex.
> - * @epf: the EPF device bound to the EPC device which has established
> - * the connection with the host
> - *
> - * Invoke to notify the function driver that EPC device has established
> - * a connection with the Root Complex.
> - */
> -void pci_epf_linkup(struct pci_epf *epf)
> -{
> - if (!epf->driver) {
> - dev_WARN(&epf->dev, "epf device not bound to driver\n");
> - return;
> - }
> -
> - epf->driver->ops->linkup(epf);
> -}
> -EXPORT_SYMBOL_GPL(pci_epf_linkup);
> -
> /**
> * pci_epf_unbind() - Notify the function driver that the binding between the
> * EPF device and EPC device has been lost
> @@ -214,7 +194,7 @@ int __pci_epf_register_driver(struct pci_epf_driver *driver,
> if (!driver->ops)
> return -EINVAL;
>
> - if (!driver->ops->bind || !driver->ops->unbind || !driver->ops->linkup)
> + if (!driver->ops->bind || !driver->ops->unbind)
> return -EINVAL;
>
> driver->driver.bus = &pci_epf_bus_type;
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 56f1846b9d39..36644ccd32ac 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -89,6 +89,7 @@ struct pci_epc_mem {
> * @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
> + * @notifier: used to notify EPF of any EPC events (like linkup)
> */
> struct pci_epc {
> struct device dev;
> @@ -99,6 +100,7 @@ struct pci_epc {
> struct config_group *group;
> /* spinlock to protect against concurrent access of EP controller */
> spinlock_t lock;
> + struct atomic_notifier_head notifier;
> };
>
> /**
> @@ -141,6 +143,12 @@ static inline void *epc_get_drvdata(struct pci_epc *epc)
> return dev_get_drvdata(&epc->dev);
> }
>
> +static inline int
> +pci_epc_register_notifier(struct pci_epc *epc, struct notifier_block *nb)
> +{
> + return atomic_notifier_chain_register(&epc->notifier, nb);
> +}
> +
> struct pci_epc *
> __devm_pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
> struct module *owner);
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 2d6f07556682..4993f7f6439b 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -55,13 +55,10 @@ struct pci_epf_header {
> * @bind: ops to perform when a EPC device has been bound to EPF device
> * @unbind: ops to perform when a binding has been lost between a EPC device
> * and EPF device
> - * @linkup: ops to perform when the EPC device has established a connection with
> - * a host system
> */
> struct pci_epf_ops {
> int (*bind)(struct pci_epf *epf);
> void (*unbind)(struct pci_epf *epf);
> - void (*linkup)(struct pci_epf *epf);
> };
>
> /**
> @@ -112,6 +109,7 @@ struct pci_epf_bar {
> * @epc: the EPC device to which this EPF device is bound
> * @driver: the EPF driver to which this EPF device is bound
> * @list: to add pci_epf as a list of PCI endpoint functions to pci_epc
> + * @nb: notifier block to notify EPF of any EPC events (like linkup)
> */
> struct pci_epf {
> struct device dev;
> @@ -125,6 +123,7 @@ struct pci_epf {
> struct pci_epc *epc;
> struct pci_epf_driver *driver;
> struct list_head list;
> + struct notifier_block nb;
> };
>
> #define to_pci_epf(epf_dev) container_of((epf_dev), struct pci_epf, dev)
> @@ -154,5 +153,4 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar);
> int pci_epf_bind(struct pci_epf *epf);
> void pci_epf_unbind(struct pci_epf *epf);
> -void pci_epf_linkup(struct pci_epf *epf);
> #endif /* __LINUX_PCI_EPF_H */
> --
> 2.17.1
>
Tested with the help of series @
http://patchwork.ozlabs.org/project/linux-pci/list/?series=158959
Tested-by: Vidya Sagar <vidyas@nvidia.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements
2020-02-12 11:25 [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Kishon Vijay Abraham I
` (4 preceding siblings ...)
2020-02-12 11:25 ` [PATCH v2 5/5] PCI: endpoint: Assign function number for each PF in EPC core Kishon Vijay Abraham I
@ 2020-02-21 15:54 ` Lorenzo Pieralisi
5 siblings, 0 replies; 14+ messages in thread
From: Lorenzo Pieralisi @ 2020-02-21 15:54 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Bjorn Helgaas, Andrew Murray, Vidya Sagar, Athani Nadeem Ladkhan,
Tom Joseph, linux-pci, linux-kernel
On Wed, Feb 12, 2020 at 04:55:09PM +0530, Kishon Vijay Abraham I wrote:
> Changes from v1:
> Rebased to Linux 5.6-rc1 and removed dependencies to my other series
> to unblock [1]
>
> [1] -> http://lore.kernel.org/r/20200103100736.27627-1-vidyas@nvidia.com
>
> v1 of this patch series can be found @
> http://lore.kernel.org/r/20191231100331.6316-1-kishon@ti.com
>
> This series adds miscellaneous improvements to PCIe endpoint core.
> 1) Protect concurrent access to memory allocation in pci-epc-mem
> 2) Replace spinlock with mutex in pci-epc-core and also use
> notification chain mechanism to notify EPC events to EPF driver.
> 3) Since endpoint function device can be created by multiple
> mechanisms (configfs, devicetree, etc..), allowing each of these
> mechanisms to assign a function number would result in mutliple
> endpoint function devices having the same function number. In order
> to avoid this, let EPC core assign a function number to the
> endpoint device.
Hi Kishon,
I am about to apply this series but I could not help notice that
some of these patches are fixes rather than improvements, it would
be good to propagate the changes to older kernels too provided
we explain the bugs in the respective logs, in particular for (3)
above it should not be complicated to reproduce a failure.
Thanks,
Lorenzo
> Kishon Vijay Abraham I (5):
> PCI: endpoint: Use notification chain mechanism to notify EPC events
> to EPF
> PCI: endpoint: Replace spinlock with mutex
> PCI: endpoint: Protect concurrent access to memory allocation with
> mutex
> PCI: endpoint: Protect concurrent access to pci_epf_ops with mutex
> PCI: endpoint: Assign function number for each PF in EPC core
>
> drivers/pci/endpoint/functions/pci-epf-test.c | 13 +-
> drivers/pci/endpoint/pci-ep-cfs.c | 27 +----
> drivers/pci/endpoint/pci-epc-core.c | 113 ++++++++----------
> drivers/pci/endpoint/pci-epc-mem.c | 10 +-
> drivers/pci/endpoint/pci-epf-core.c | 33 ++---
> include/linux/pci-epc.h | 19 ++-
> include/linux/pci-epf.h | 9 +-
> 7 files changed, 108 insertions(+), 116 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
2020-02-12 11:25 ` [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex Kishon Vijay Abraham I
@ 2021-06-11 9:52 ` Vidya Sagar
2021-06-20 13:21 ` Vidya Sagar
2021-06-21 5:14 ` Kishon Vijay Abraham I
0 siblings, 2 replies; 14+ messages in thread
From: Vidya Sagar @ 2021-06-11 9:52 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: linux-pci, linux-kernel, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Athani Nadeem Ladkhan, Tom Joseph,
Manikanta Maddireddy, Om Prakash Singh, Krishna Thota
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
2021-06-11 9:52 ` Vidya Sagar
@ 2021-06-20 13:21 ` Vidya Sagar
2021-06-21 5:14 ` Kishon Vijay Abraham I
1 sibling, 0 replies; 14+ messages in thread
From: Vidya Sagar @ 2021-06-20 13:21 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: linux-pci, linux-kernel, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Athani Nadeem Ladkhan, Tom Joseph,
Manikanta Maddireddy, Om Prakash Singh, Krishna Thota
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
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
2021-06-11 9:52 ` Vidya Sagar
2021-06-20 13:21 ` Vidya Sagar
@ 2021-06-21 5:14 ` Kishon Vijay Abraham I
2021-06-21 9:38 ` Vidya Sagar
1 sibling, 1 reply; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2021-06-21 5:14 UTC (permalink / raw)
To: Vidya Sagar
Cc: linux-pci, linux-kernel, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Athani Nadeem Ladkhan, Tom Joseph,
Manikanta Maddireddy, Om Prakash Singh, Krishna Thota
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
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
2021-06-21 5:14 ` Kishon Vijay Abraham I
@ 2021-06-21 9:38 ` Vidya Sagar
2021-06-21 13:37 ` Kishon Vijay Abraham I
0 siblings, 1 reply; 14+ messages in thread
From: Vidya Sagar @ 2021-06-21 9:38 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: linux-pci, linux-kernel, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Athani Nadeem Ladkhan, Tom Joseph,
Manikanta Maddireddy, Om Prakash Singh, Krishna Thota
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
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
2021-06-21 9:38 ` Vidya Sagar
@ 2021-06-21 13:37 ` Kishon Vijay Abraham I
2021-06-22 5:29 ` Vidya Sagar
0 siblings, 1 reply; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2021-06-21 13:37 UTC (permalink / raw)
To: Vidya Sagar
Cc: linux-pci, linux-kernel, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Athani Nadeem Ladkhan, Tom Joseph,
Manikanta Maddireddy, Om Prakash Singh, Krishna Thota
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
>>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex
2021-06-21 13:37 ` Kishon Vijay Abraham I
@ 2021-06-22 5:29 ` Vidya Sagar
0 siblings, 0 replies; 14+ messages in thread
From: Vidya Sagar @ 2021-06-22 5:29 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: linux-pci, linux-kernel, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Athani Nadeem Ladkhan, Tom Joseph,
Manikanta Maddireddy, Om Prakash Singh, Krishna Thota
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
>>>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-22 5:30 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 11:25 [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 1/5] PCI: endpoint: Use notification chain mechanism to notify EPC events to EPF Kishon Vijay Abraham I
2020-02-17 12:14 ` Vidya Sagar
2020-02-12 11:25 ` [PATCH v2 2/5] PCI: endpoint: Replace spinlock with mutex Kishon Vijay Abraham I
2021-06-11 9:52 ` Vidya Sagar
2021-06-20 13:21 ` Vidya Sagar
2021-06-21 5:14 ` Kishon Vijay Abraham I
2021-06-21 9:38 ` Vidya Sagar
2021-06-21 13:37 ` Kishon Vijay Abraham I
2021-06-22 5:29 ` Vidya Sagar
2020-02-12 11:25 ` [PATCH v2 3/5] PCI: endpoint: Protect concurrent access to memory allocation " Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 4/5] PCI: endpoint: Protect concurrent access to pci_epf_ops " Kishon Vijay Abraham I
2020-02-12 11:25 ` [PATCH v2 5/5] PCI: endpoint: Assign function number for each PF in EPC core Kishon Vijay Abraham I
2020-02-21 15:54 ` [PATCH v2 0/5] PCI: Endpoint: Miscellaneous improvements Lorenzo Pieralisi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.