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

[Manivannan Sadhasivam]

HI Kishon,

On Tue, Oct 11, 2022 at 06:27:37PM +0530, Kishon Vijay Abraham I wrote:
> Hi Mani,
> 
> On 06/10/22 7:19 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@xxxxxxxxxx>
> > ---
> >   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;
> 
> Doesn't this ignore epc_features input from the controller driver?

Sorry I don't get it! epc_features from the controller is acuquired during
pci_epf_test_bind(). EPF probe doesn't have any visibility of the controller
driver.

> > +
> >   	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..ba54f17ae06f 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->core_init)
> 
> This would result in abort if the endpoint function driver is not bound to
> endpoint device and the notify is called.
> 

I don't think so. We are iterating the "epc->pci_epf" list, which will only be
populated if a EPF driver gets bind to an endpoint device.

> This would also require all function drivers to have event_ops populated.
> IOW this could break pci-epf-ntb.c.
> 

This I missed. I will add a check for the "epf->event_ops" existence first.

Thanks,
Mani

> Thanks,
> Kishon

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