From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Niklas Cassel <cassel@kernel.org>
Cc: "Lorenzo Pieralisi" <lpieralisi@kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Rob Herring" <robh@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Kishon Vijay Abraham I" <kishon@kernel.org>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Jonathan Hunter" <jonathanh@nvidia.com>,
"Jingoo Han" <jingoohan1@gmail.com>,
"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org, mhi@lists.linux.dev,
linux-tegra@vger.kernel.org
Subject: Re: [PATCH 02/11] PCI: endpoint: Decouple EPC and PCIe bus specific events
Date: Tue, 26 Mar 2024 13:19:07 +0530 [thread overview]
Message-ID: <20240326074907.GD9565@thinkpad> (raw)
In-Reply-To: <Zf2tBNctPpIrHeWA@ryzen>
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@linaro.org>
> > ---
> > 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 prev parent reply other threads:[~2024-03-26 7:49 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-14 15:23 [PATCH 00/11] PCI: endpoint: Make host reboot handling more robust Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert Manivannan Sadhasivam
2024-03-22 16:08 ` Niklas Cassel
2024-03-26 7:44 ` Manivannan Sadhasivam
2024-03-26 10:24 ` Niklas Cassel
2024-03-26 11:10 ` Manivannan Sadhasivam
2024-03-26 13:47 ` Niklas Cassel
2024-03-26 13:55 ` Manivannan Sadhasivam
2024-03-26 10:37 ` Niklas Cassel
2024-03-14 15:23 ` [PATCH 02/11] PCI: endpoint: Decouple EPC and PCIe bus specific events Manivannan Sadhasivam
2024-03-22 16:08 ` Niklas Cassel
2024-03-26 7:49 ` Manivannan Sadhasivam [this message]
2024-03-14 15:23 ` [PATCH 03/11] PCI: endpoint: Rename core_init() callback in 'struct pci_epc_event_ops' to init() Manivannan Sadhasivam
2024-03-22 16:08 ` Niklas Cassel
2024-03-26 7:56 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 04/11] PCI: epf-test: Refactor pci_epf_test_unbind() function Manivannan Sadhasivam
2024-03-22 16:09 ` Niklas Cassel
2024-03-26 7:58 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-26 8:26 ` Manivannan Sadhasivam
2024-03-26 11:05 ` Niklas Cassel
2024-03-26 14:27 ` Niklas Cassel
2024-03-27 6:20 ` Manivannan Sadhasivam
2024-03-27 6:18 ` Manivannan Sadhasivam
2024-03-27 11:39 ` Niklas Cassel
2024-03-28 18:42 ` Vinod Koul
2024-04-04 8:44 ` Niklas Cassel
2024-04-22 7:55 ` Manivannan Sadhasivam
2024-04-22 9:30 ` Niklas Cassel
2024-03-14 15:23 ` [PATCH 06/11] PCI: endpoint: Introduce EPC 'deinit' event and notify the EPF drivers Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-26 8:31 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 07/11] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle Link Down event Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-27 18:06 ` Niklas Cassel
2024-04-01 16:34 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 08/11] PCI: qcom-ep: Use the " Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 09/11] PCI: epf-test: Handle " Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-14 15:23 ` [PATCH 10/11] PCI: qcom-ep: Rework {start/stop}_link() callbacks implementation Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-26 8:33 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 11/11] PCI: tegra194: " Manivannan Sadhasivam
2024-03-22 16:11 ` Niklas Cassel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240326074907.GD9565@thinkpad \
--to=manivannan.sadhasivam@linaro.org \
--cc=bhelgaas@google.com \
--cc=cassel@kernel.org \
--cc=gustavo.pimentel@synopsys.com \
--cc=jingoohan1@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=kishon@kernel.org \
--cc=kw@linux.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=lpieralisi@kernel.org \
--cc=mhi@lists.linux.dev \
--cc=robh@kernel.org \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).