[PATCH v4 1/2] PCI: add helper function to find root port for device

[PATCH v4 1/2] PCI: add helper function to find root port for device

[Lucas Stach]

This adds a simple way to get the root port a given device
is connected to.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
v2: new patch in v2
v3: rename to pci_find_rootport to fit better with other API
v4: - rename to make it obvious that this function is PCIe specific
    - fixes wrong assumption about what is a root bus in the presence
      virtual buses
---
 drivers/pci/search.c | 20 ++++++++++++++++++++
 include/linux/pci.h  |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d5e2a7..d7c599103ae1 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
 	return 0;
 }
 EXPORT_SYMBOL(pci_dev_present);
+
+/**
+ * pcie_find_root_port - Returns the root port the given device is connected to.
+ * @dev: PCI device for which the root port should be found.
+ */
+struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+
+	/* if this device is located on the root bus, it is a root port */
+	if (pci_is_root_bus(bus))
+		return dev;
+
+	/* walk up the PCI hierarchy to the first level below the root */
+	while (bus->parent && bus->parent->parent)
+		bus = bus->parent;
+
+	return bus->self;
+}
+EXPORT_SYMBOL(pcie_find_root_port);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 211e9da8a7d7..308c71081034 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -844,6 +844,7 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 }
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
 int pci_dev_present(const struct pci_device_id *ids);
+struct pci_dev *pcie_find_root_port(struct pci_dev *dev);
 
 int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
 			     int where, u8 *val);
-- 
2.1.4

[PATCH v4 1/2] PCI: add helper function to find root port for device

[Lucas Stach]

This adds a simple way to get the root port a given device
is connected to.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
v2: new patch in v2
v3: rename to pci_find_rootport to fit better with other API
v4: - rename to make it obvious that this function is PCIe specific
    - fixes wrong assumption about what is a root bus in the presence
      virtual buses
---
 drivers/pci/search.c | 20 ++++++++++++++++++++
 include/linux/pci.h  |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d5e2a7..d7c599103ae1 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
 	return 0;
 }
 EXPORT_SYMBOL(pci_dev_present);
+
+/**
+ * pcie_find_root_port - Returns the root port the given device is connected to.
+ * @dev: PCI device for which the root port should be found.
+ */
+struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+
+	/* if this device is located on the root bus, it is a root port */
+	if (pci_is_root_bus(bus))
+		return dev;
+
+	/* walk up the PCI hierarchy to the first level below the root */
+	while (bus->parent && bus->parent->parent)
+		bus = bus->parent;
+
+	return bus->self;
+}
+EXPORT_SYMBOL(pcie_find_root_port);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 211e9da8a7d7..308c71081034 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -844,6 +844,7 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 }
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
 int pci_dev_present(const struct pci_device_id *ids);
+struct pci_dev *pcie_find_root_port(struct pci_dev *dev);
 
 int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
 			     int where, u8 *val);
-- 
2.1.4

[PATCH v4 2/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

The fixup to enable relaxed ordering on all PCI devices was
executed unconditionally if the Tegra PCI host driver was
built into the kernel. This doesn't play nice with a
multiplatform kernel executed on other platforms which
may not need this fixup.

Make sure to only apply the fixup if the root port is
a Tegra.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2:
- split out PCI hierarchy walk
- separate code from data by moving PCI IDs into own structure
v3:
- fixup for helper rename
- applied Thierry's ACK
v4:
- fixup for yet another helper rename
---
 drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 00e92720d7f7..cc3b1ed77754 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -586,10 +586,42 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_fixup_class);
 
+static const struct pci_device_id tegra_rootport_ids[] = {
+	{
+		/* Tegra20 4 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0bf0,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra20 2 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0bf1,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra30 4 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e1c,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra30 2 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e1d,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra124 4 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e12,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra124 1 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e13,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* sentinel */
+	}
+};
+
 /* Tegra PCIE requires relaxed ordering */
 static void tegra_pcie_relax_enable(struct pci_dev *dev)
 {
-	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
+	if (pci_match_id(tegra_rootport_ids, pcie_find_root_port(dev)))
+		pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
+					 PCI_EXP_DEVCTL_RELAX_EN);
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
 
-- 
2.1.4

[PATCH v4 2/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

The fixup to enable relaxed ordering on all PCI devices was
executed unconditionally if the Tegra PCI host driver was
built into the kernel. This doesn't play nice with a
multiplatform kernel executed on other platforms which
may not need this fixup.

Make sure to only apply the fixup if the root port is
a Tegra.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Acked-by: Thierry Reding <treding@nvidia.com>
---
v2:
- split out PCI hierarchy walk
- separate code from data by moving PCI IDs into own structure
v3:
- fixup for helper rename
- applied Thierry's ACK
v4:
- fixup for yet another helper rename
---
 drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 00e92720d7f7..cc3b1ed77754 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -586,10 +586,42 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_fixup_class);
 
+static const struct pci_device_id tegra_rootport_ids[] = {
+	{
+		/* Tegra20 4 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0bf0,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra20 2 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0bf1,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra30 4 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e1c,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra30 2 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e1d,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra124 4 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e12,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* Tegra124 1 lane root port */
+		.vendor = PCI_VENDOR_ID_NVIDIA, .device = 0x0e13,
+		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+	}, {
+		/* sentinel */
+	}
+};
+
 /* Tegra PCIE requires relaxed ordering */
 static void tegra_pcie_relax_enable(struct pci_dev *dev)
 {
-	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
+	if (pci_match_id(tegra_rootport_ids, pcie_find_root_port(dev)))
+		pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
+					 PCI_EXP_DEVCTL_RELAX_EN);
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
 
-- 
2.1.4

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Alex Williamson]

On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> This adds a simple way to get the root port a given device
> is connected to.
> 
> Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> v2: new patch in v2
> v3: rename to pci_find_rootport to fit better with other API
> v4: - rename to make it obvious that this function is PCIe specific
>     - fixes wrong assumption about what is a root bus in the presence
>       virtual buses
> ---
>  drivers/pci/search.c | 20 ++++++++++++++++++++
>  include/linux/pci.h  |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index a20ce7d5e2a7..d7c599103ae1 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
>  	return 0;
>  }
>  EXPORT_SYMBOL(pci_dev_present);
> +
> +/**
> + * pcie_find_root_port - Returns the root port the given device is connected to.
> + * @dev: PCI device for which the root port should be found.
> + */
> +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +
> +	/* if this device is located on the root bus, it is a root port */
> +	if (pci_is_root_bus(bus))
> +		return dev;

It could also be a root complex endpoint or a conventional PCI
device/bridge sitting on the host bridge bus.

> +
> +	/* walk up the PCI hierarchy to the first level below the root */
> +	while (bus->parent && bus->parent->parent)
> +		bus = bus->parent;
> +
> +	return bus->self;
> +}
> +EXPORT_SYMBOL(pcie_find_root_port);

IMHO, this makes too many assumptions about the topology that it's
working with for a generic interface.  Your usage may be fairly fixed,
but there are too many cases where it could return something that's not
a root port as a general interface.  Thanks,

Alex

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 211e9da8a7d7..308c71081034 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -844,6 +844,7 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
>  }
>  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
>  int pci_dev_present(const struct pci_device_id *ids);
> +struct pci_dev *pcie_find_root_port(struct pci_dev *dev);
>  
>  int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
>  			     int where, u8 *val);

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Alex Williamson]

On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> This adds a simple way to get the root port a given device
> is connected to.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> v2: new patch in v2
> v3: rename to pci_find_rootport to fit better with other API
> v4: - rename to make it obvious that this function is PCIe specific
>     - fixes wrong assumption about what is a root bus in the presence
>       virtual buses
> ---
>  drivers/pci/search.c | 20 ++++++++++++++++++++
>  include/linux/pci.h  |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index a20ce7d5e2a7..d7c599103ae1 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
>  	return 0;
>  }
>  EXPORT_SYMBOL(pci_dev_present);
> +
> +/**
> + * pcie_find_root_port - Returns the root port the given device is connected to.
> + * @dev: PCI device for which the root port should be found.
> + */
> +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +
> +	/* if this device is located on the root bus, it is a root port */
> +	if (pci_is_root_bus(bus))
> +		return dev;

It could also be a root complex endpoint or a conventional PCI
device/bridge sitting on the host bridge bus.

> +
> +	/* walk up the PCI hierarchy to the first level below the root */
> +	while (bus->parent && bus->parent->parent)
> +		bus = bus->parent;
> +
> +	return bus->self;
> +}
> +EXPORT_SYMBOL(pcie_find_root_port);

IMHO, this makes too many assumptions about the topology that it's
working with for a generic interface.  Your usage may be fairly fixed,
but there are too many cases where it could return something that's not
a root port as a general interface.  Thanks,

Alex

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 211e9da8a7d7..308c71081034 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -844,6 +844,7 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
>  }
>  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
>  int pci_dev_present(const struct pci_device_id *ids);
> +struct pci_dev *pcie_find_root_port(struct pci_dev *dev);
>  
>  int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
>  			     int where, u8 *val);

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Lucas Stach]

Am Montag, den 30.03.2015, 08:33 -0600 schrieb Alex Williamson:
> On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> > This adds a simple way to get the root port a given device
> > is connected to.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> > v2: new patch in v2
> > v3: rename to pci_find_rootport to fit better with other API
> > v4: - rename to make it obvious that this function is PCIe specific
> >     - fixes wrong assumption about what is a root bus in the presence
> >       virtual buses
> > ---
> >  drivers/pci/search.c | 20 ++++++++++++++++++++
> >  include/linux/pci.h  |  1 +
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > index a20ce7d5e2a7..d7c599103ae1 100644
> > --- a/drivers/pci/search.c
> > +++ b/drivers/pci/search.c
> > @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(pci_dev_present);
> > +
> > +/**
> > + * pcie_find_root_port - Returns the root port the given device is connected to.
> > + * @dev: PCI device for which the root port should be found.
> > + */
> > +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> > +{
> > +	struct pci_bus *bus = dev->bus;
> > +
> > +	/* if this device is located on the root bus, it is a root port */
> > +	if (pci_is_root_bus(bus))
> > +		return dev;
> 
> It could also be a root complex endpoint or a conventional PCI
> device/bridge sitting on the host bridge bus.

> > +
> > +	/* walk up the PCI hierarchy to the first level below the root */
> > +	while (bus->parent && bus->parent->parent)
> > +		bus = bus->parent;
> > +
> > +	return bus->self;
> > +}
> > +EXPORT_SYMBOL(pcie_find_root_port);
> 
> IMHO, this makes too many assumptions about the topology that it's
> working with for a generic interface.  Your usage may be fairly fixed,
> but there are too many cases where it could return something that's not
> a root port as a general interface.  Thanks,
> 
I'm open to suggestions on how to improve the detection. I really need
something which works reliable in the majority of cases, as the Tegra
quirk should not be executed on other platforms.

Do you think filtering out EP devices and conventional PCI bridges on
the root bus is enough?

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Alex Williamson]

On Mon, 2015-03-30 at 18:06 +0200, Lucas Stach wrote:
> Am Montag, den 30.03.2015, 08:33 -0600 schrieb Alex Williamson:
> > On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> > > This adds a simple way to get the root port a given device
> > > is connected to.
> > > 
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > > v2: new patch in v2
> > > v3: rename to pci_find_rootport to fit better with other API
> > > v4: - rename to make it obvious that this function is PCIe specific
> > >     - fixes wrong assumption about what is a root bus in the presence
> > >       virtual buses
> > > ---
> > >  drivers/pci/search.c | 20 ++++++++++++++++++++
> > >  include/linux/pci.h  |  1 +
> > >  2 files changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > index a20ce7d5e2a7..d7c599103ae1 100644
> > > --- a/drivers/pci/search.c
> > > +++ b/drivers/pci/search.c
> > > @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL(pci_dev_present);
> > > +
> > > +/**
> > > + * pcie_find_root_port - Returns the root port the given device is connected to.
> > > + * @dev: PCI device for which the root port should be found.
> > > + */
> > > +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> > > +{
> > > +	struct pci_bus *bus = dev->bus;
> > > +
> > > +	/* if this device is located on the root bus, it is a root port */
> > > +	if (pci_is_root_bus(bus))
> > > +		return dev;
> > 
> > It could also be a root complex endpoint or a conventional PCI
> > device/bridge sitting on the host bridge bus.
> 
> > > +
> > > +	/* walk up the PCI hierarchy to the first level below the root */
> > > +	while (bus->parent && bus->parent->parent)
> > > +		bus = bus->parent;
> > > +
> > > +	return bus->self;
> > > +}
> > > +EXPORT_SYMBOL(pcie_find_root_port);
> > 
> > IMHO, this makes too many assumptions about the topology that it's
> > working with for a generic interface.  Your usage may be fairly fixed,
> > but there are too many cases where it could return something that's not
> > a root port as a general interface.  Thanks,
> > 
> I'm open to suggestions on how to improve the detection. I really need
> something which works reliable in the majority of cases, as the Tegra
> quirk should not be executed on other platforms.
> 
> Do you think filtering out EP devices and conventional PCI bridges on
> the root bus is enough?

I'm actually pretty confused by the implementation of the quirk as well,
why do you even need this pcie_find_root_port() function?  Your fixup is
called for every single PCI device in the system, so why do you need to
go to the trouble of scanning the topology for the root port?  You'll be
passed the root port eventually and it will match your ID table w/o any
extra effort.  As coded, you're calling the set capability function
multiple times per root port, once for itself and once for each device
below it.

Personally, I'd probably do away with the table, declare a fixup for
each entry for the specific vendor/device ID, and make a simple quirk
callback that sets the capability bit.  Just my preference though.
Thanks,

Alex

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Lucas Stach]

Am Montag, den 30.03.2015, 10:52 -0600 schrieb Alex Williamson:
> On Mon, 2015-03-30 at 18:06 +0200, Lucas Stach wrote:
> > Am Montag, den 30.03.2015, 08:33 -0600 schrieb Alex Williamson:
> > > On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> > > > This adds a simple way to get the root port a given device
> > > > is connected to.
> > > > 
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > ---
> > > > v2: new patch in v2
> > > > v3: rename to pci_find_rootport to fit better with other API
> > > > v4: - rename to make it obvious that this function is PCIe specific
> > > >     - fixes wrong assumption about what is a root bus in the presence
> > > >       virtual buses
> > > > ---
> > > >  drivers/pci/search.c | 20 ++++++++++++++++++++
> > > >  include/linux/pci.h  |  1 +
> > > >  2 files changed, 21 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > > index a20ce7d5e2a7..d7c599103ae1 100644
> > > > --- a/drivers/pci/search.c
> > > > +++ b/drivers/pci/search.c
> > > > @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
> > > >  	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL(pci_dev_present);
> > > > +
> > > > +/**
> > > > + * pcie_find_root_port - Returns the root port the given device is connected to.
> > > > + * @dev: PCI device for which the root port should be found.
> > > > + */
> > > > +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> > > > +{
> > > > +	struct pci_bus *bus = dev->bus;
> > > > +
> > > > +	/* if this device is located on the root bus, it is a root port */
> > > > +	if (pci_is_root_bus(bus))
> > > > +		return dev;
> > > 
> > > It could also be a root complex endpoint or a conventional PCI
> > > device/bridge sitting on the host bridge bus.
> > 
> > > > +
> > > > +	/* walk up the PCI hierarchy to the first level below the root */
> > > > +	while (bus->parent && bus->parent->parent)
> > > > +		bus = bus->parent;
> > > > +
> > > > +	return bus->self;
> > > > +}
> > > > +EXPORT_SYMBOL(pcie_find_root_port);
> > > 
> > > IMHO, this makes too many assumptions about the topology that it's
> > > working with for a generic interface.  Your usage may be fairly fixed,
> > > but there are too many cases where it could return something that's not
> > > a root port as a general interface.  Thanks,
> > > 
> > I'm open to suggestions on how to improve the detection. I really need
> > something which works reliable in the majority of cases, as the Tegra
> > quirk should not be executed on other platforms.
> > 
> > Do you think filtering out EP devices and conventional PCI bridges on
> > the root bus is enough?
> 
> I'm actually pretty confused by the implementation of the quirk as well,
> why do you even need this pcie_find_root_port() function?  Your fixup is
> called for every single PCI device in the system, so why do you need to
> go to the trouble of scanning the topology for the root port?  You'll be
> passed the root port eventually and it will match your ID table w/o any
> extra effort.  As coded, you're calling the set capability function
> multiple times per root port, once for itself and once for each device
> below it.
> 
> Personally, I'd probably do away with the table, declare a fixup for
> each entry for the specific vendor/device ID, and make a simple quirk
> callback that sets the capability bit.  Just my preference though.
> Thanks,
> 
No, you missed the point of the fixup here.

We need to apply this fixup on every device in the system if the root
port is a Tegra. So the fixup needs to get called for every device,
filtering on a specific vendor/device ID is not possible.

But on the other hand this fixup may be compiled into a multiplatform
kernel. If this kernel is strated on a device which isn't a Tegra (and
so has no Tegra root port) we don't want to apply the fixup at all.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Alex Williamson]

On Tue, 2015-03-31 at 14:23 +0200, Lucas Stach wrote:
> Am Montag, den 30.03.2015, 10:52 -0600 schrieb Alex Williamson:
> > On Mon, 2015-03-30 at 18:06 +0200, Lucas Stach wrote:
> > > Am Montag, den 30.03.2015, 08:33 -0600 schrieb Alex Williamson:
> > > > On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> > > > > This adds a simple way to get the root port a given device
> > > > > is connected to.
> > > > > 
> > > > > Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > ---
> > > > > v2: new patch in v2
> > > > > v3: rename to pci_find_rootport to fit better with other API
> > > > > v4: - rename to make it obvious that this function is PCIe specific
> > > > >     - fixes wrong assumption about what is a root bus in the presence
> > > > >       virtual buses
> > > > > ---
> > > > >  drivers/pci/search.c | 20 ++++++++++++++++++++
> > > > >  include/linux/pci.h  |  1 +
> > > > >  2 files changed, 21 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > > > index a20ce7d5e2a7..d7c599103ae1 100644
> > > > > --- a/drivers/pci/search.c
> > > > > +++ b/drivers/pci/search.c
> > > > > @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
> > > > >  	return 0;
> > > > >  }
> > > > >  EXPORT_SYMBOL(pci_dev_present);
> > > > > +
> > > > > +/**
> > > > > + * pcie_find_root_port - Returns the root port the given device is connected to.
> > > > > + * @dev: PCI device for which the root port should be found.
> > > > > + */
> > > > > +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> > > > > +{
> > > > > +	struct pci_bus *bus = dev->bus;
> > > > > +
> > > > > +	/* if this device is located on the root bus, it is a root port */
> > > > > +	if (pci_is_root_bus(bus))
> > > > > +		return dev;
> > > > 
> > > > It could also be a root complex endpoint or a conventional PCI
> > > > device/bridge sitting on the host bridge bus.
> > > 
> > > > > +
> > > > > +	/* walk up the PCI hierarchy to the first level below the root */
> > > > > +	while (bus->parent && bus->parent->parent)
> > > > > +		bus = bus->parent;
> > > > > +
> > > > > +	return bus->self;
> > > > > +}
> > > > > +EXPORT_SYMBOL(pcie_find_root_port);
> > > > 
> > > > IMHO, this makes too many assumptions about the topology that it's
> > > > working with for a generic interface.  Your usage may be fairly fixed,
> > > > but there are too many cases where it could return something that's not
> > > > a root port as a general interface.  Thanks,
> > > > 
> > > I'm open to suggestions on how to improve the detection. I really need
> > > something which works reliable in the majority of cases, as the Tegra
> > > quirk should not be executed on other platforms.
> > > 
> > > Do you think filtering out EP devices and conventional PCI bridges on
> > > the root bus is enough?
> > 
> > I'm actually pretty confused by the implementation of the quirk as well,
> > why do you even need this pcie_find_root_port() function?  Your fixup is
> > called for every single PCI device in the system, so why do you need to
> > go to the trouble of scanning the topology for the root port?  You'll be
> > passed the root port eventually and it will match your ID table w/o any
> > extra effort.  As coded, you're calling the set capability function
> > multiple times per root port, once for itself and once for each device
> > below it.
> > 
> > Personally, I'd probably do away with the table, declare a fixup for
> > each entry for the specific vendor/device ID, and make a simple quirk
> > callback that sets the capability bit.  Just my preference though.
> > Thanks,
> > 
> No, you missed the point of the fixup here.
> 
> We need to apply this fixup on every device in the system if the root
> port is a Tegra. So the fixup needs to get called for every device,
> filtering on a specific vendor/device ID is not possible.
> 
> But on the other hand this fixup may be compiled into a multiplatform
> kernel. If this kernel is strated on a device which isn't a Tegra (and
> so has no Tegra root port) we don't want to apply the fixup at all.

Yes, you're right, sorry for the misinterpretation.  So the quirk works
as expected, but I think at a minimum the helper function needs to be
renamed to something like pci_find_root_bus_dev() since there is no
attempt made to verify whether the returned device is actually a root
port.  If you don't support hotplug, you could also apply the quirk from
the root port down instead of searching from every device up.  Thanks,

Alex

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Alex Williamson]

On Tue, 2015-03-31 at 14:23 +0200, Lucas Stach wrote:
> Am Montag, den 30.03.2015, 10:52 -0600 schrieb Alex Williamson:
> > On Mon, 2015-03-30 at 18:06 +0200, Lucas Stach wrote:
> > > Am Montag, den 30.03.2015, 08:33 -0600 schrieb Alex Williamson:
> > > > On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> > > > > This adds a simple way to get the root port a given device
> > > > > is connected to.
> > > > > 
> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > > ---
> > > > > v2: new patch in v2
> > > > > v3: rename to pci_find_rootport to fit better with other API
> > > > > v4: - rename to make it obvious that this function is PCIe specific
> > > > >     - fixes wrong assumption about what is a root bus in the presence
> > > > >       virtual buses
> > > > > ---
> > > > >  drivers/pci/search.c | 20 ++++++++++++++++++++
> > > > >  include/linux/pci.h  |  1 +
> > > > >  2 files changed, 21 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > > > index a20ce7d5e2a7..d7c599103ae1 100644
> > > > > --- a/drivers/pci/search.c
> > > > > +++ b/drivers/pci/search.c
> > > > > @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
> > > > >  	return 0;
> > > > >  }
> > > > >  EXPORT_SYMBOL(pci_dev_present);
> > > > > +
> > > > > +/**
> > > > > + * pcie_find_root_port - Returns the root port the given device is connected to.
> > > > > + * @dev: PCI device for which the root port should be found.
> > > > > + */
> > > > > +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> > > > > +{
> > > > > +	struct pci_bus *bus = dev->bus;
> > > > > +
> > > > > +	/* if this device is located on the root bus, it is a root port */
> > > > > +	if (pci_is_root_bus(bus))
> > > > > +		return dev;
> > > > 
> > > > It could also be a root complex endpoint or a conventional PCI
> > > > device/bridge sitting on the host bridge bus.
> > > 
> > > > > +
> > > > > +	/* walk up the PCI hierarchy to the first level below the root */
> > > > > +	while (bus->parent && bus->parent->parent)
> > > > > +		bus = bus->parent;
> > > > > +
> > > > > +	return bus->self;
> > > > > +}
> > > > > +EXPORT_SYMBOL(pcie_find_root_port);
> > > > 
> > > > IMHO, this makes too many assumptions about the topology that it's
> > > > working with for a generic interface.  Your usage may be fairly fixed,
> > > > but there are too many cases where it could return something that's not
> > > > a root port as a general interface.  Thanks,
> > > > 
> > > I'm open to suggestions on how to improve the detection. I really need
> > > something which works reliable in the majority of cases, as the Tegra
> > > quirk should not be executed on other platforms.
> > > 
> > > Do you think filtering out EP devices and conventional PCI bridges on
> > > the root bus is enough?
> > 
> > I'm actually pretty confused by the implementation of the quirk as well,
> > why do you even need this pcie_find_root_port() function?  Your fixup is
> > called for every single PCI device in the system, so why do you need to
> > go to the trouble of scanning the topology for the root port?  You'll be
> > passed the root port eventually and it will match your ID table w/o any
> > extra effort.  As coded, you're calling the set capability function
> > multiple times per root port, once for itself and once for each device
> > below it.
> > 
> > Personally, I'd probably do away with the table, declare a fixup for
> > each entry for the specific vendor/device ID, and make a simple quirk
> > callback that sets the capability bit.  Just my preference though.
> > Thanks,
> > 
> No, you missed the point of the fixup here.
> 
> We need to apply this fixup on every device in the system if the root
> port is a Tegra. So the fixup needs to get called for every device,
> filtering on a specific vendor/device ID is not possible.
> 
> But on the other hand this fixup may be compiled into a multiplatform
> kernel. If this kernel is strated on a device which isn't a Tegra (and
> so has no Tegra root port) we don't want to apply the fixup at all.

Yes, you're right, sorry for the misinterpretation.  So the quirk works
as expected, but I think at a minimum the helper function needs to be
renamed to something like pci_find_root_bus_dev() since there is no
attempt made to verify whether the returned device is actually a root
port.  If you don't support hotplug, you could also apply the quirk from
the root port down instead of searching from every device up.  Thanks,

Alex

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Bjorn Helgaas]

On Tue, Mar 31, 2015 at 08:09:53AM -0600, Alex Williamson wrote:
> On Tue, 2015-03-31 at 14:23 +0200, Lucas Stach wrote:
> > Am Montag, den 30.03.2015, 10:52 -0600 schrieb Alex Williamson:
> > > On Mon, 2015-03-30 at 18:06 +0200, Lucas Stach wrote:
> > > > Am Montag, den 30.03.2015, 08:33 -0600 schrieb Alex Williamson:
> > > > > On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> > > > > > This adds a simple way to get the root port a given device
> > > > > > is connected to.
> > > > > > 
> > > > > > Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > > ---
> > > > > > v2: new patch in v2
> > > > > > v3: rename to pci_find_rootport to fit better with other API
> > > > > > v4: - rename to make it obvious that this function is PCIe specific
> > > > > >     - fixes wrong assumption about what is a root bus in the presence
> > > > > >       virtual buses
> > > > > > ---
> > > > > >  drivers/pci/search.c | 20 ++++++++++++++++++++
> > > > > >  include/linux/pci.h  |  1 +
> > > > > >  2 files changed, 21 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > > > > index a20ce7d5e2a7..d7c599103ae1 100644
> > > > > > --- a/drivers/pci/search.c
> > > > > > +++ b/drivers/pci/search.c
> > > > > > @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(pci_dev_present);
> > > > > > +
> > > > > > +/**
> > > > > > + * pcie_find_root_port - Returns the root port the given device is connected to.
> > > > > > + * @dev: PCI device for which the root port should be found.
> > > > > > + */
> > > > > > +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> > > > > > +{
> > > > > > +	struct pci_bus *bus = dev->bus;
> > > > > > +
> > > > > > +	/* if this device is located on the root bus, it is a root port */
> > > > > > +	if (pci_is_root_bus(bus))
> > > > > > +		return dev;
> > > > > 
> > > > > It could also be a root complex endpoint or a conventional PCI
> > > > > device/bridge sitting on the host bridge bus.
> > > > 
> > > > > > +
> > > > > > +	/* walk up the PCI hierarchy to the first level below the root */
> > > > > > +	while (bus->parent && bus->parent->parent)
> > > > > > +		bus = bus->parent;
> > > > > > +
> > > > > > +	return bus->self;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(pcie_find_root_port);
> > > > > 
> > > > > IMHO, this makes too many assumptions about the topology that it's
> > > > > working with for a generic interface.  Your usage may be fairly fixed,
> > > > > but there are too many cases where it could return something that's not
> > > > > a root port as a general interface.  Thanks,
> > > > > 
> > > > I'm open to suggestions on how to improve the detection. I really need
> > > > something which works reliable in the majority of cases, as the Tegra
> > > > quirk should not be executed on other platforms.
> > > > 
> > > > Do you think filtering out EP devices and conventional PCI bridges on
> > > > the root bus is enough?
> > > 
> > > I'm actually pretty confused by the implementation of the quirk as well,
> > > why do you even need this pcie_find_root_port() function?  Your fixup is
> > > called for every single PCI device in the system, so why do you need to
> > > go to the trouble of scanning the topology for the root port?  You'll be
> > > passed the root port eventually and it will match your ID table w/o any
> > > extra effort.  As coded, you're calling the set capability function
> > > multiple times per root port, once for itself and once for each device
> > > below it.
> > > 
> > > Personally, I'd probably do away with the table, declare a fixup for
> > > each entry for the specific vendor/device ID, and make a simple quirk
> > > callback that sets the capability bit.  Just my preference though.
> > > Thanks,
> > > 
> > No, you missed the point of the fixup here.
> > 
> > We need to apply this fixup on every device in the system if the root
> > port is a Tegra. So the fixup needs to get called for every device,
> > filtering on a specific vendor/device ID is not possible.
> > 
> > But on the other hand this fixup may be compiled into a multiplatform
> > kernel. If this kernel is strated on a device which isn't a Tegra (and
> > so has no Tegra root port) we don't want to apply the fixup at all.
> 
> Yes, you're right, sorry for the misinterpretation.  So the quirk works
> as expected, but I think at a minimum the helper function needs to be
> renamed to something like pci_find_root_bus_dev() since there is no
> attempt made to verify whether the returned device is actually a root
> port.  If you don't support hotplug, you could also apply the quirk from
> the root port down instead of searching from every device up.  Thanks,

How should we proceed here?  I agree with Alex's comments.

Bjorn

Re: [PATCH v4 1/2] PCI: add helper function to find root port for device

[Bjorn Helgaas]

On Tue, Mar 31, 2015 at 08:09:53AM -0600, Alex Williamson wrote:
> On Tue, 2015-03-31 at 14:23 +0200, Lucas Stach wrote:
> > Am Montag, den 30.03.2015, 10:52 -0600 schrieb Alex Williamson:
> > > On Mon, 2015-03-30 at 18:06 +0200, Lucas Stach wrote:
> > > > Am Montag, den 30.03.2015, 08:33 -0600 schrieb Alex Williamson:
> > > > > On Mon, 2015-03-30 at 12:55 +0200, Lucas Stach wrote:
> > > > > > This adds a simple way to get the root port a given device
> > > > > > is connected to.
> > > > > > 
> > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > > > ---
> > > > > > v2: new patch in v2
> > > > > > v3: rename to pci_find_rootport to fit better with other API
> > > > > > v4: - rename to make it obvious that this function is PCIe specific
> > > > > >     - fixes wrong assumption about what is a root bus in the presence
> > > > > >       virtual buses
> > > > > > ---
> > > > > >  drivers/pci/search.c | 20 ++++++++++++++++++++
> > > > > >  include/linux/pci.h  |  1 +
> > > > > >  2 files changed, 21 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > > > > index a20ce7d5e2a7..d7c599103ae1 100644
> > > > > > --- a/drivers/pci/search.c
> > > > > > +++ b/drivers/pci/search.c
> > > > > > @@ -384,3 +384,23 @@ int pci_dev_present(const struct pci_device_id *ids)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(pci_dev_present);
> > > > > > +
> > > > > > +/**
> > > > > > + * pcie_find_root_port - Returns the root port the given device is connected to.
> > > > > > + * @dev: PCI device for which the root port should be found.
> > > > > > + */
> > > > > > +struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
> > > > > > +{
> > > > > > +	struct pci_bus *bus = dev->bus;
> > > > > > +
> > > > > > +	/* if this device is located on the root bus, it is a root port */
> > > > > > +	if (pci_is_root_bus(bus))
> > > > > > +		return dev;
> > > > > 
> > > > > It could also be a root complex endpoint or a conventional PCI
> > > > > device/bridge sitting on the host bridge bus.
> > > > 
> > > > > > +
> > > > > > +	/* walk up the PCI hierarchy to the first level below the root */
> > > > > > +	while (bus->parent && bus->parent->parent)
> > > > > > +		bus = bus->parent;
> > > > > > +
> > > > > > +	return bus->self;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(pcie_find_root_port);
> > > > > 
> > > > > IMHO, this makes too many assumptions about the topology that it's
> > > > > working with for a generic interface.  Your usage may be fairly fixed,
> > > > > but there are too many cases where it could return something that's not
> > > > > a root port as a general interface.  Thanks,
> > > > > 
> > > > I'm open to suggestions on how to improve the detection. I really need
> > > > something which works reliable in the majority of cases, as the Tegra
> > > > quirk should not be executed on other platforms.
> > > > 
> > > > Do you think filtering out EP devices and conventional PCI bridges on
> > > > the root bus is enough?
> > > 
> > > I'm actually pretty confused by the implementation of the quirk as well,
> > > why do you even need this pcie_find_root_port() function?  Your fixup is
> > > called for every single PCI device in the system, so why do you need to
> > > go to the trouble of scanning the topology for the root port?  You'll be
> > > passed the root port eventually and it will match your ID table w/o any
> > > extra effort.  As coded, you're calling the set capability function
> > > multiple times per root port, once for itself and once for each device
> > > below it.
> > > 
> > > Personally, I'd probably do away with the table, declare a fixup for
> > > each entry for the specific vendor/device ID, and make a simple quirk
> > > callback that sets the capability bit.  Just my preference though.
> > > Thanks,
> > > 
> > No, you missed the point of the fixup here.
> > 
> > We need to apply this fixup on every device in the system if the root
> > port is a Tegra. So the fixup needs to get called for every device,
> > filtering on a specific vendor/device ID is not possible.
> > 
> > But on the other hand this fixup may be compiled into a multiplatform
> > kernel. If this kernel is strated on a device which isn't a Tegra (and
> > so has no Tegra root port) we don't want to apply the fixup at all.
> 
> Yes, you're right, sorry for the misinterpretation.  So the quirk works
> as expected, but I think at a minimum the helper function needs to be
> renamed to something like pci_find_root_bus_dev() since there is no
> attempt made to verify whether the returned device is actually a root
> port.  If you don't support hotplug, you could also apply the quirk from
> the root port down instead of searching from every device up.  Thanks,

How should we proceed here?  I agree with Alex's comments.

Bjorn