Re: [PATCH 02/11] PCI: endpoint: Decouple EPC and PCIe bus specific events

From: Manivannan Sadhasivam
Date: Tue Mar 26 2024 - 03:49:36 EST

Next message: Heiko Carstens: "Re: [PATCH 1/1] mm: vmalloc: Bail out early in find_vmap_area() if vmap is not init"
Previous message: Michael S. Tsirkin: "Re: [PATCH] virtio_ring: Fix the stale index in available ring"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Mar 22, 2024 at 05:08:36PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:41PM +0530, Manivannan Sadhasivam wrote:
> > Currently, 'struct pci_epc_event_ops' has a bunch of events that are sent
> > from the EPC driver to EPF driver. But those events are a mix of EPC
> > specific events like core_init and PCIe bus specific events like LINK_UP,
> > LINK_DOWN, BME etc...
> >
> > Let's decouple them to respective structs (pci_epc_event_ops,
> > pci_epc_bus_event_ops) to make the separation clear.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > drivers/pci/endpoint/functions/pci-epf-mhi.c | 8 ++++++--
> > drivers/pci/endpoint/functions/pci-epf-test.c | 8 ++++++--
> > drivers/pci/endpoint/pci-epc-core.c | 20 ++++++++++----------
> > include/linux/pci-epf.h | 23 ++++++++++++++++-------
> > 4 files changed, 38 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > index 1c3e4ea76bd2..e5d67aec7574 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > @@ -880,8 +880,11 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
> > pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no, epf_bar);
> > }
> >
> > -static const struct pci_epc_event_ops pci_epf_mhi_event_ops = {
> > +static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
> > .core_init = pci_epf_mhi_core_init,
> > +};
> > +
> > +static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
> > .link_up = pci_epf_mhi_link_up,
> > .link_down = pci_epf_mhi_link_down,
> > .bme = pci_epf_mhi_bme,
> > @@ -903,7 +906,8 @@ static int pci_epf_mhi_probe(struct pci_epf *epf,
> > epf_mhi->info = info;
> > epf_mhi->epf = epf;
> >
> > - epf->event_ops = &pci_epf_mhi_event_ops;
> > + epf->epc_event_ops = &pci_epf_mhi_epc_event_ops;
> > + epf->bus_event_ops = &pci_epf_mhi_bus_event_ops;
> >
> > mutex_init(&epf_mhi->lock);
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index fc0282b0d626..751dab5799d5 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -813,8 +813,11 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
> > return 0;
> > }
> >
> > -static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> > +static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
> > .core_init = pci_epf_test_core_init,
> > +};
> > +
> > +static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
> > .link_up = pci_epf_test_link_up,
> > };
>
> I'm not a big fan of every EPF driver now needing two different
> static const struct pci_*_event_ops.
>
> Is really:
> static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
> .core_init = pci_epf_test_core_init,
> };
>
> static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
> .link_up = pci_epf_test_link_up,
> };
>
>
> Better than:
> 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,
> }
>
> The callbacks should have sufficiently distinct names that it is obvious
> what it is happening?
>
> Link up is that the EPC driver tells me that it is link up.
> Init is that the EPF function should initialize the BARs etc.
>
> I'm not saying that I'm totally against this, but I'm not sure that there
> are so many EPC callbacks that this is needed?
>

The issue I'm seeing is that these callbacks are serving different purposes. One
is purely EPC specific and another is PCIe Link specific. So mixing them in a
single struct doesn't look good IMO.

And I agree that we will be left with 2 structs, but at least I can see that it
gives a clear representation of the purposes of the callbacks.

- Mani

> How many will there be after this series?
> Four? .init, .deinit, .link_up, .link_down ?
>
> I would vote to keep all callbacks in the same struct for now,
> but you are the maintainer.
>
>
> >
> > @@ -959,7 +962,8 @@ static int pci_epf_test_probe(struct pci_epf *epf,
> >
> > INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);
> >
> > - epf->event_ops = &pci_epf_test_event_ops;
> > + epf->epc_event_ops = &pci_epf_test_epc_event_ops;
> > + epf->bus_event_ops = &pci_epf_test_bus_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 ba2ff037dfa6..f602f08a11a2 100644
> > --- a/drivers/pci/endpoint/pci-epc-core.c
> > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > @@ -697,8 +697,8 @@ void pci_epc_linkup(struct pci_epc *epc)
> > 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->link_up)
> > - epf->event_ops->link_up(epf);
> > + if (epf->bus_event_ops && epf->bus_event_ops->link_up)
> > + epf->bus_event_ops->link_up(epf);
> > mutex_unlock(&epf->lock);
> > }
> > mutex_unlock(&epc->list_lock);
> > @@ -723,8 +723,8 @@ void pci_epc_linkdown(struct pci_epc *epc)
> > 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->link_down)
> > - epf->event_ops->link_down(epf);
> > + if (epf->bus_event_ops && epf->bus_event_ops->link_down)
> > + epf->bus_event_ops->link_down(epf);
> > mutex_unlock(&epf->lock);
> > }
> > mutex_unlock(&epc->list_lock);
> > @@ -749,8 +749,8 @@ void pci_epc_init_notify(struct pci_epc *epc)
> > 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);
> > + if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> > + epf->epc_event_ops->core_init(epf);
> > mutex_unlock(&epf->lock);
> > }
> > epc->init_complete = true;
> > @@ -772,8 +772,8 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
> > {
> > if (epc->init_complete) {
> > mutex_lock(&epf->lock);
> > - if (epf->event_ops && epf->event_ops->core_init)
> > - epf->event_ops->core_init(epf);
> > + if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> > + epf->epc_event_ops->core_init(epf);
> > mutex_unlock(&epf->lock);
> > }
> > }
> > @@ -797,8 +797,8 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > 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->bme)
> > - epf->event_ops->bme(epf);
> > + if (epf->bus_event_ops && epf->bus_event_ops->bme)
> > + epf->bus_event_ops->bme(epf);
> > mutex_unlock(&epf->lock);
> > }
> > mutex_unlock(&epc->list_lock);
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 77b146e0f672..1271e1e00bbd 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -68,14 +68,21 @@ struct pci_epf_ops {
> > };
> >
> > /**
> > - * struct pci_epc_event_ops - Callbacks for capturing the EPC events
> > - * @core_init: Callback for the EPC initialization complete event
> > - * @link_up: Callback for the EPC link up event
> > - * @link_down: Callback for the EPC link down event
> > - * @bme: Callback for the EPC BME (Bus Master Enable) event
> > + * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
> > + * @core_init: Callback for the EPC initialization event
> > */
> > struct pci_epc_event_ops {
> > int (*core_init)(struct pci_epf *epf);
> > +};
> > +
> > +/**
> > + * struct pci_epc_bus_event_ops - Callbacks for capturing the PCIe bus specific
> > + * events
> > + * @link_up: Callback for the PCIe bus link up event
> > + * @link_down: Callback for the PCIe bus link down event
> > + * @bme: Callback for the PCIe bus BME (Bus Master Enable) event
> > + */
> > +struct pci_epc_bus_event_ops {
> > int (*link_up)(struct pci_epf *epf);
> > int (*link_down)(struct pci_epf *epf);
> > int (*bme)(struct pci_epf *epf);
> > @@ -149,7 +156,8 @@ 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
> > + * @epc_event_ops: Callbacks for capturing the EPC events
> > + * @bus_event_ops: Callbacks for capturing the PCIe bus events
> > */
> > struct pci_epf {
> > struct device dev;
> > @@ -179,7 +187,8 @@ 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;
> > + const struct pci_epc_event_ops *epc_event_ops;
> > + const struct pci_epc_bus_event_ops *bus_event_ops;
> > };
> >
> > /**
> >
> > --
> > 2.25.1
> >
>

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

Next message: Heiko Carstens: "Re: [PATCH 1/1] mm: vmalloc: Bail out early in find_vmap_area() if vmap is not init"
Previous message: Michael S. Tsirkin: "Re: [PATCH] virtio_ring: Fix the stale index in available ring"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]