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

[Kishon Vijay Abraham I]

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@xxxxxxxxxx>
> ---
>  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