Re: [PATCH v2 3/3] PCI: endpoint: Use link_up() callback in place of LINK_UP notifier

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

-- 
மணிவண்ணன் சதாசிவம்