| 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: <20230529171024.GF5633@thinkpad>
Date: Mon, 29 May 2023 22:40:24 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
kw@...ux.com, kishon@...nel.org, bhelgaas@...gle.com,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v4 1/7] PCI: endpoint: Pass EPF device ID to the probe
function
On Mon, May 29, 2023 at 11:36:03AM +0200, Lorenzo Pieralisi wrote:
> On Fri, May 19, 2023 at 08:12:09PM +0530, Manivannan Sadhasivam wrote:
> > Currently, the EPF probe function doesn't get the device ID argument needed
> > to correctly identify the device table ID of the EPF device.
> >
> > When multiple entries are added to the "struct pci_epf_device_id" table,
> > the probe function needs to identify the correct one. And the only way to
> > do so is by storing the correct device ID in "struct pci_epf" during
> > "pci_epf_match_id()" and passing that to probe().
> >
> > Reviewed-by: Kishon Vijay Abraham I <kishon@...nel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > ---
> > drivers/pci/endpoint/functions/pci-epf-ntb.c | 3 ++-
> > drivers/pci/endpoint/functions/pci-epf-test.c | 2 +-
> > drivers/pci/endpoint/functions/pci-epf-vntb.c | 2 +-
> > drivers/pci/endpoint/pci-epf-core.c | 8 +++++---
> > include/linux/pci-epf.h | 4 +++-
> > 5 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> > index 9a00448c7e61..980b4ecf19a2 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-ntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> > @@ -2075,11 +2075,12 @@ static struct config_group *epf_ntb_add_cfs(struct pci_epf *epf,
> > /**
> > * epf_ntb_probe() - Probe NTB function driver
> > * @epf: NTB endpoint function device
> > + * @id: NTB endpoint function device ID
> > *
> > * Probe NTB function driver when endpoint function bus detects a NTB
> > * endpoint function.
> > */
> > -static int epf_ntb_probe(struct pci_epf *epf)
> > +static int epf_ntb_probe(struct pci_epf *epf, const struct pci_epf_device_id *id)
> > {
> > struct epf_ntb *ntb;
> > struct device *dev;
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 0f9d2ec822ac..d5fcc78a5b73 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -980,7 +980,7 @@ static const struct pci_epf_device_id pci_epf_test_ids[] = {
> > {},
> > };
> >
> > -static int pci_epf_test_probe(struct pci_epf *epf)
> > +static int pci_epf_test_probe(struct pci_epf *epf, const struct pci_epf_device_id *id)
> > {
> > struct pci_epf_test *epf_test;
> > struct device *dev = &epf->dev;
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index b7c7a8af99f4..122eb7a12028 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -1401,7 +1401,7 @@ static struct pci_epf_ops epf_ntb_ops = {
> > *
> > * Returns: Zero for success, or an error code in case of failure
> > */
> > -static int epf_ntb_probe(struct pci_epf *epf)
> > +static int epf_ntb_probe(struct pci_epf *epf, const struct pci_epf_device_id *id)
> > {
> > struct epf_ntb *ntb;
> > struct device *dev;
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index 2036e38be093..924564288c9a 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -494,11 +494,13 @@ static const struct device_type pci_epf_type = {
> > };
> >
> > static int
> > -pci_epf_match_id(const struct pci_epf_device_id *id, const struct pci_epf *epf)
> > +pci_epf_match_id(const struct pci_epf_device_id *id, struct pci_epf *epf)
> > {
> > while (id->name[0]) {
> > - if (strcmp(epf->name, id->name) == 0)
> > + if (strcmp(epf->name, id->name) == 0) {
> > + epf->id = id;
> > return true;
> > + }
> > id++;
> > }
>
> I disagree with this patch's intent. The match function should not
> change the parameters state. We should export this function to drivers
> so that upon probe they can retrieve the matching id themselves,
> as other bus interfaces do IMO.
>
Ok, if you do not want to change the parameters inside match function, then I
could move the matching part to a separate function and make it to return the
correct pci_epf_device_id pointer. Based on this, the match() function can
return bool if the match is found or not and the pci_epf_device_probe() function
can pass the returned id to the driver's probe function. This is what being done
for PCI driver as well [1].
- Mani
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-driver.c#n415
> Thanks,
> Lorenzo
>
> > @@ -526,7 +528,7 @@ static int pci_epf_device_probe(struct device *dev)
> >
> > epf->driver = driver;
> >
> > - return driver->probe(epf);
> > + return driver->probe(epf, epf->id);
> > }
> >
> > static void pci_epf_device_remove(struct device *dev)
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index a215dc8ce693..bc613f0df7e3 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -89,7 +89,7 @@ struct pci_epc_event_ops {
> > * @id_table: identifies EPF devices for probing
> > */
> > struct pci_epf_driver {
> > - int (*probe)(struct pci_epf *epf);
> > + int (*probe)(struct pci_epf *epf, const struct pci_epf_device_id *id);
> > void (*remove)(struct pci_epf *epf);
> >
> > struct device_driver driver;
> > @@ -131,6 +131,7 @@ struct pci_epf_bar {
> > * @epc: the EPC device to which this EPF device is bound
> > * @epf_pf: the physical EPF device to which this virtual EPF device is bound
> > * @driver: the EPF driver to which this EPF device is bound
> > + * @id: Pointer to the EPF device ID
> > * @list: to add pci_epf as a list of PCI endpoint functions to pci_epc
> > * @lock: mutex to protect pci_epf_ops
> > * @sec_epc: the secondary EPC device to which this EPF device is bound
> > @@ -158,6 +159,7 @@ struct pci_epf {
> > struct pci_epc *epc;
> > struct pci_epf *epf_pf;
> > struct pci_epf_driver *driver;
> > + const struct pci_epf_device_id *id;
> > struct list_head list;
> > /* mutex to protect against concurrent access of pci_epf_ops */
> > struct mutex lock;
> > --
> > 2.25.1
> >
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists