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
--
மணிவண்ணன் சதாசிவம்
