| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <45b2db99-2d03-469b-aa37-bc6c63cef141@kernel.org>
Date: Tue, 2 Apr 2024 09:14:20 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
Kishon Vijay Abraham I <kishon@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>, Jingoo Han <jingoohan1@...il.com>
Cc: linux-pci@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, mhi@...ts.linux.dev,
linux-tegra@...r.kernel.org, Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v2 02/10] PCI: endpoint: Decouple EPC and PCIe bus
specific events
On 4/2/24 00:50, 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.
I fail to see the benefits here. The event operation names are quite clear and,
in my opinion, it is clear if an event op applies to the controller or to the
bus/link. If anything, "core_init" could a little more clear, so renaming that
"ep_controller_init" or something like that (clearly spelling out what is being
initialized) seems enough to me. Similarly, the "bme" op name is very criptic.
Renaming that to "bus_master_enable" would go a long way clarifying the code.
For link events, "link_up", "link_down" are clear. So I think there is no need
to split the event op struct like this. Renaming the ops is better.
Note that I am not opposed to this patch, but I think it is just code churn
that does not really bring any fundamental improvement. Regardless, renaming
"core_init" and "bme" ops is I think desired.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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 2c54d80107cf..280863c0eeb9 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -896,8 +896,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,
> @@ -919,7 +922,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 977fb79c1567..973db0b1bde2 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -798,8 +798,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,
> };
>
> @@ -916,7 +919,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 47d27ec7439d..f202ae07ffa9 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -692,8 +692,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);
> @@ -718,8 +718,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);
> @@ -744,8 +744,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;
> @@ -767,8 +767,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);
> }
> }
> @@ -792,8 +792,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 adee6a1b35db..77399fecaeb5 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -69,14 +69,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);
> @@ -150,7 +157,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;
> @@ -180,7 +188,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;
> };
>
> /**
>
--
Damien Le Moal
Western Digital Research
Powered by blists - more mailing lists