| Message ID | 20220910090508.61157-4-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: endpoint: Rework the EPC to EPF notification | expand |
Hi Mani, On 10/09/22 14:35, Manivannan Sadhasivam wrote: > As a part of the transition towards callback mechanism for signalling the > events from EPC to EPF, let's use the link_up() callback in the place of > the LINK_UP notifier. This also removes the notifier support completely > from the PCI endpoint framework. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/endpoint/functions/pci-epf-test.c | 33 ++++++------------- > drivers/pci/endpoint/pci-epc-core.c | 12 +++++-- > include/linux/pci-epc.h | 8 ----- > include/linux/pci-epf.h | 8 ++--- > 4 files changed, 22 insertions(+), 39 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index 868de17e1ad2..f75045f2dee3 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -826,30 +826,21 @@ static int pci_epf_test_core_init(struct pci_epf *epf) > return 0; > } > > -static const struct pci_epc_event_ops pci_epf_test_event_ops = { > - .core_init = pci_epf_test_core_init, > -}; > - > -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val, > - void *data) > +int pci_epf_test_link_up(struct pci_epf *epf) > { > - struct pci_epf *epf = container_of(nb, struct pci_epf, nb); > struct pci_epf_test *epf_test = epf_get_drvdata(epf); > > - switch (val) { > - case LINK_UP: > - queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler, > - msecs_to_jiffies(1)); > - break; > - > - default: > - dev_err(&epf->dev, "Invalid EPF test notifier event\n"); > - return NOTIFY_BAD; > - } > + queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler, > + msecs_to_jiffies(1)); > > - return NOTIFY_OK; > + return 0; > } > > +static const struct pci_epc_event_ops pci_epf_test_event_ops = { > + .core_init = pci_epf_test_core_init, > + .link_up = pci_epf_test_link_up, > +}; > + > static int pci_epf_test_alloc_space(struct pci_epf *epf) > { > struct pci_epf_test *epf_test = epf_get_drvdata(epf); > @@ -976,12 +967,8 @@ static int pci_epf_test_bind(struct pci_epf *epf) > if (ret) > epf_test->dma_supported = false; > > - if (linkup_notifier || core_init_notifier) { > - epf->nb.notifier_call = pci_epf_test_notifier; > - pci_epc_register_notifier(epc, &epf->nb); > - } else { > + if (!linkup_notifier && !core_init_notifier) > queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work); > - } > > return 0; > } > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c > index ba54f17ae06f..5dac1496cf16 100644 > --- a/drivers/pci/endpoint/pci-epc-core.c > +++ b/drivers/pci/endpoint/pci-epc-core.c > @@ -690,10 +690,19 @@ EXPORT_SYMBOL_GPL(pci_epc_remove_epf); > */ > void pci_epc_linkup(struct pci_epc *epc) > { > + struct pci_epf *epf; > + > if (!epc || IS_ERR(epc)) > return; > > - atomic_notifier_call_chain(&epc->notifier, LINK_UP, NULL); > + mutex_lock(&epc->list_lock); This will break pci-dra7xx which invokes pci_epc_linkup() in interrupt context. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-dra7xx.c#n332 dra7xx_pcie_irq_handler() | | dw_pcie_ep_linkup() | | pci_epc_linkup() | | mutex_lock() Thanks, Kishon
Hi Kishon, On Mon, Sep 19, 2022 at 02:28:33PM +0530, Kishon Vijay Abraham I wrote: > Hi Mani, > > On 10/09/22 14:35, Manivannan Sadhasivam wrote: > > As a part of the transition towards callback mechanism for signalling the > > events from EPC to EPF, let's use the link_up() callback in the place of > > the LINK_UP notifier. This also removes the notifier support completely > > from the PCI endpoint framework. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/endpoint/functions/pci-epf-test.c | 33 ++++++------------- > > drivers/pci/endpoint/pci-epc-core.c | 12 +++++-- > > include/linux/pci-epc.h | 8 ----- > > include/linux/pci-epf.h | 8 ++--- > > 4 files changed, 22 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > index 868de17e1ad2..f75045f2dee3 100644 > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > @@ -826,30 +826,21 @@ static int pci_epf_test_core_init(struct pci_epf *epf) > > return 0; > > } > > > > -static const struct pci_epc_event_ops pci_epf_test_event_ops = { > > - .core_init = pci_epf_test_core_init, > > -}; > > - > > -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val, > > - void *data) > > +int pci_epf_test_link_up(struct pci_epf *epf) > > { > > - struct pci_epf *epf = container_of(nb, struct pci_epf, nb); > > struct pci_epf_test *epf_test = epf_get_drvdata(epf); > > > > - switch (val) { > > - case LINK_UP: > > - queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler, > > - msecs_to_jiffies(1)); > > - break; > > - > > - default: > > - dev_err(&epf->dev, "Invalid EPF test notifier event\n"); > > - return NOTIFY_BAD; > > - } > > + queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler, > > + msecs_to_jiffies(1)); > > > > - return NOTIFY_OK; > > + return 0; > > } > > > > +static const struct pci_epc_event_ops pci_epf_test_event_ops = { > > + .core_init = pci_epf_test_core_init, > > + .link_up = pci_epf_test_link_up, > > +}; > > + > > static int pci_epf_test_alloc_space(struct pci_epf *epf) > > { > > struct pci_epf_test *epf_test = epf_get_drvdata(epf); > > @@ -976,12 +967,8 @@ static int pci_epf_test_bind(struct pci_epf *epf) > > if (ret) > > epf_test->dma_supported = false; > > > > - if (linkup_notifier || core_init_notifier) { > > - epf->nb.notifier_call = pci_epf_test_notifier; > > - pci_epc_register_notifier(epc, &epf->nb); > > - } else { > > + if (!linkup_notifier && !core_init_notifier) > > queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work); > > - } > > > > return 0; > > } > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c > > index ba54f17ae06f..5dac1496cf16 100644 > > --- a/drivers/pci/endpoint/pci-epc-core.c > > +++ b/drivers/pci/endpoint/pci-epc-core.c > > @@ -690,10 +690,19 @@ EXPORT_SYMBOL_GPL(pci_epc_remove_epf); > > */ > > void pci_epc_linkup(struct pci_epc *epc) > > { > > + struct pci_epf *epf; > > + > > if (!epc || IS_ERR(epc)) > > return; > > > > - atomic_notifier_call_chain(&epc->notifier, LINK_UP, NULL); > > + mutex_lock(&epc->list_lock); > > This will break pci-dra7xx which invokes pci_epc_linkup() in interrupt > context. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-dra7xx.c#n332 > Ah, sorry missed this one. > dra7xx_pcie_irq_handler() > | > | > dw_pcie_ep_linkup() This doesn't look right to me. IRQ handlers are supposed to execute quickly and not block for long time. Here the EPF drivers are free to consume more time if they want and that will block the IRQ handler. Since the IRQ handler is just reporting the IRQs and calling link_up handler, it looks like it should be made as a threaded IRQ as like other DWC drivers. Thoughts? Thanks, Mani > | > | > pci_epc_linkup() > | > | > mutex_lock() > > Thanks, > Kishon
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index 868de17e1ad2..f75045f2dee3 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -826,30 +826,21 @@ static int pci_epf_test_core_init(struct pci_epf *epf) return 0; } -static const struct pci_epc_event_ops pci_epf_test_event_ops = { - .core_init = pci_epf_test_core_init, -}; - -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val, - void *data) +int pci_epf_test_link_up(struct pci_epf *epf) { - struct pci_epf *epf = container_of(nb, struct pci_epf, nb); struct pci_epf_test *epf_test = epf_get_drvdata(epf); - switch (val) { - case LINK_UP: - queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler, - msecs_to_jiffies(1)); - break; - - default: - dev_err(&epf->dev, "Invalid EPF test notifier event\n"); - return NOTIFY_BAD; - } + queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler, + msecs_to_jiffies(1)); - return NOTIFY_OK; + return 0; } +static const struct pci_epc_event_ops pci_epf_test_event_ops = { + .core_init = pci_epf_test_core_init, + .link_up = pci_epf_test_link_up, +}; + static int pci_epf_test_alloc_space(struct pci_epf *epf) { struct pci_epf_test *epf_test = epf_get_drvdata(epf); @@ -976,12 +967,8 @@ static int pci_epf_test_bind(struct pci_epf *epf) if (ret) epf_test->dma_supported = false; - if (linkup_notifier || core_init_notifier) { - epf->nb.notifier_call = pci_epf_test_notifier; - pci_epc_register_notifier(epc, &epf->nb); - } else { + if (!linkup_notifier && !core_init_notifier) queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work); - } return 0; } diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c index ba54f17ae06f..5dac1496cf16 100644 --- a/drivers/pci/endpoint/pci-epc-core.c +++ b/drivers/pci/endpoint/pci-epc-core.c @@ -690,10 +690,19 @@ EXPORT_SYMBOL_GPL(pci_epc_remove_epf); */ void pci_epc_linkup(struct pci_epc *epc) { + struct pci_epf *epf; + if (!epc || IS_ERR(epc)) return; - atomic_notifier_call_chain(&epc->notifier, LINK_UP, NULL); + mutex_lock(&epc->list_lock); + list_for_each_entry(epf, &epc->pci_epf, list) { + mutex_lock(&epf->lock); + if (epf->event_ops->link_up) + epf->event_ops->link_up(epf); + mutex_unlock(&epf->lock); + } + mutex_unlock(&epc->list_lock); } EXPORT_SYMBOL_GPL(pci_epc_linkup); @@ -784,7 +793,6 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops, mutex_init(&epc->lock); mutex_init(&epc->list_lock); INIT_LIST_HEAD(&epc->pci_epf); - ATOMIC_INIT_NOTIFIER_HEAD(&epc->notifier); device_initialize(&epc->dev); epc->dev.class = pci_epc_class; diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h index fe729dfe509b..301bb0e53707 100644 --- a/include/linux/pci-epc.h +++ b/include/linux/pci-epc.h @@ -135,7 +135,6 @@ struct pci_epc_mem { * @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 { struct device dev; @@ -151,7 +150,6 @@ struct pci_epc { /* mutex to protect against concurrent access of EP controller */ struct mutex lock; unsigned long function_num_map; - struct atomic_notifier_head notifier; }; /** @@ -194,12 +192,6 @@ 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 a06f3b4c8bee..bc613f0df7e3 100644 --- a/include/linux/pci-epf.h +++ b/include/linux/pci-epf.h @@ -17,10 +17,6 @@ struct pci_epf; enum pci_epc_interface_type; -enum pci_notify_event { - LINK_UP, -}; - enum pci_barno { NO_BAR = -1, BAR_0, @@ -74,9 +70,11 @@ struct pci_epf_ops { /** * struct pci_epf_event_ops - Callbacks for capturing the EPC events * @core_init: Callback for the EPC initialization complete event + * @link_up: Callback for the EPC link up event */ struct pci_epc_event_ops { int (*core_init)(struct pci_epf *epf); + int (*link_up)(struct pci_epf *epf); }; /** @@ -135,7 +133,6 @@ struct pci_epf_bar { * @driver: the EPF driver to which this EPF device is bound * @id: Pointer to the EPF device ID * @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 * @sec_epc: the secondary EPC device to which this EPF device is bound * @sec_epc_list: to add pci_epf as list of PCI endpoint functions to secondary @@ -164,7 +161,6 @@ struct pci_epf { struct pci_epf_driver *driver; const struct pci_epf_device_id *id; struct list_head list; - struct notifier_block nb; /* mutex to protect against concurrent access of pci_epf_ops */ struct mutex lock;
As a part of the transition towards callback mechanism for signalling the events from EPC to EPF, let's use the link_up() callback in the place of the LINK_UP notifier. This also removes the notifier support completely from the PCI endpoint framework. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/endpoint/functions/pci-epf-test.c | 33 ++++++------------- drivers/pci/endpoint/pci-epc-core.c | 12 +++++-- include/linux/pci-epc.h | 8 ----- include/linux/pci-epf.h | 8 ++--- 4 files changed, 22 insertions(+), 39 deletions(-)