Re: [PATCH v4 4/5] PCI: endpoint: Use callback mechanism for passing events from EPC to EPF

[Kishon Vijay Abraham I <kvijayab@amd.com>]

On 10/25/2022 8:21 PM, Manivannan Sadhasivam wrote:
> Instead of using the notifiers for passing the events from EPC to EPF,
> let's introduce a callback based mechanism where the EPF drivers can
> populate relevant callbacks for EPC events they want to subscribe.
> 
> The use of notifiers in kernel is not recommended if there is a real link
> between the sender and receiver, like in this case. Also, the existing
> atomic notifier forces the notification functions to be in atomic context
> while the caller may be in non-atomic context. For instance, the two
> in-kernel users of the notifiers, pcie-qcom and pcie-tegra194, both are
> calling the notifier functions in non-atomic context (from threaded IRQ
> handlers). This creates a sleeping in atomic context issue with the
> existing EPF_TEST driver that calls the EPC APIs that may sleep.
> 
> For all these reasons, let's get rid of the notifier chains and use the
> simple callback mechanism for signalling the events from EPC to EPF
> drivers. This preserves the context of the caller and avoids the latency
> of going through a separate interface for triggering the notifications.
> 
> As a first step of the transition, the core_init() callback is introduced
> in this commit, that'll replace the existing CORE_INIT notifier used for
> signalling the init complete event from EPC.
> 
> During the occurrence of the event, EPC will go over the list of EPF
> drivers attached to it and will call the core_init() callback if available.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Acked-by: Kishon Vijay Abraham I <kishon@kernel.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 13 ++++++-------
>  drivers/pci/endpoint/pci-epc-core.c           | 11 ++++++++++-
>  include/linux/pci-epf.h                       | 11 ++++++++++-
>  3 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index a6f906a96669..868de17e1ad2 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -826,20 +826,17 @@ 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)
>  {
>  	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> -	int ret;
>  
>  	switch (val) {
> -	case CORE_INIT:
> -		ret = pci_epf_test_core_init(epf);
> -		if (ret)
> -			return NOTIFY_BAD;
> -		break;
> -
>  	case LINK_UP:
>  		queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>  				   msecs_to_jiffies(1));
> @@ -1010,6 +1007,8 @@ static int pci_epf_test_probe(struct pci_epf *epf, const struct pci_epf_device_i
>  
>  	INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);
>  
> +	epf->event_ops = &pci_epf_test_event_ops;
> +
>  	epf_set_drvdata(epf, epf_test);
>  	return 0;
>  }
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 6cce430d431b..8bb60528f530 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -707,10 +707,19 @@ EXPORT_SYMBOL_GPL(pci_epc_linkup);
>   */
>  void pci_epc_init_notify(struct pci_epc *epc)
>  {
> +	struct pci_epf *epf;
> +
>  	if (!epc || IS_ERR(epc))
>  		return;
>  
> -	atomic_notifier_call_chain(&epc->notifier, CORE_INIT, NULL);
> +	mutex_lock(&epc->list_lock);
> +	list_for_each_entry(epf, &epc->pci_epf, list) {
> +		mutex_lock(&epf->lock);
> +		if (epf->event_ops && epf->event_ops->core_init)
> +			epf->event_ops->core_init(epf);
> +		mutex_unlock(&epf->lock);
> +	}
> +	mutex_unlock(&epc->list_lock);
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_init_notify);
>  
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 0c94cc1513bc..a06f3b4c8bee 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -18,7 +18,6 @@ struct pci_epf;
>  enum pci_epc_interface_type;
>  
>  enum pci_notify_event {
> -	CORE_INIT,
>  	LINK_UP,
>  };
>  
> @@ -72,6 +71,14 @@ struct pci_epf_ops {
>  					struct config_group *group);
>  };
>  
> +/**
> + * struct pci_epf_event_ops - Callbacks for capturing the EPC events
> + * @core_init: Callback for the EPC initialization complete event
> + */
> +struct pci_epc_event_ops {
> +	int (*core_init)(struct pci_epf *epf);
> +};
> +
>  /**
>   * struct pci_epf_driver - represents the PCI EPF driver
>   * @probe: ops to perform when a new EPF device has been bound to the EPF driver
> @@ -140,6 +147,7 @@ struct pci_epf_bar {
>   * @is_vf: true - virtual function, false - physical function
>   * @vfunction_num_map: bitmap to manage virtual function number
>   * @pci_vepf: list of virtual endpoint functions associated with this function
> + * @event_ops: Callbacks for capturing the EPC events
>   */
>  struct pci_epf {
>  	struct device		dev;
> @@ -170,6 +178,7 @@ struct pci_epf {
>  	unsigned int		is_vf;
>  	unsigned long		vfunction_num_map;
>  	struct list_head	pci_vepf;
> +	const struct pci_epc_event_ops *event_ops;
>  };
>  
>  /**