[1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

Message ID 20180804101402.10022-2-hch@lst.de
State New
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • [1/3] PCI: add a callback to struct pci_host_bridge for adding a new device
Related show

Commit Message

Christoph Hellwig Aug. 4, 2018, 10:14 a.m.
There is currently no way for a PCIe bridge to impose constraints on
devices added to it.  For example, the Xilinx PCIe host bridge only
supports 32-bit physical addresses (due to a limitation on the AXI
port's address width).  Thus, even devices that claim to support 64-bit
DMA addresses must be restricted to 32-bit addresses when attached to
this host controller.

This patch adds a "add_device" method  to struct pci_host_bridge that
allows the host driver to act upon acting adding devices.

Based on an earlier patch from Wesley W. Terpstra <wesley@sifive.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/probe.c | 6 ++++++
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+)

Comments

Lorenzo Pieralisi Aug. 6, 2018, 11:23 a.m. | #1
On Sat, Aug 04, 2018 at 12:14:00PM +0200, Christoph Hellwig wrote:
> There is currently no way for a PCIe bridge to impose constraints on
> devices added to it.  For example, the Xilinx PCIe host bridge only
> supports 32-bit physical addresses (due to a limitation on the AXI
> port's address width).  Thus, even devices that claim to support 64-bit
> DMA addresses must be restricted to 32-bit addresses when attached to
> this host controller.
> 
> This patch adds a "add_device" method  to struct pci_host_bridge that
> allows the host driver to act upon acting adding devices.
> 
> Based on an earlier patch from Wesley W. Terpstra <wesley@sifive.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/probe.c | 6 ++++++
>  include/linux/pci.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e32de4b..452190fb05e7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2290,6 +2290,7 @@ static void pci_set_msi_domain(struct pci_dev *dev)
>  
>  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  {
> +	struct pci_host_bridge *host = pci_find_host_bridge(bus);
>  	int ret;
>  
>  	pci_configure_device(dev);
> @@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  	ret = pcibios_add_device(dev);
>  	WARN_ON(ret < 0);
>  
> +	if (host->add_device) {
> +		ret = host->add_device(dev);
> +		WARN_ON(ret < 0);
> +	}

This looks fine; we could go a step further and make the hunk above
the default (weak) implementation of pcibios_add_device() that is
currently a NOP returning 0, I will remove it for v4.20.

I am fine with the patch as-is (even though I am not a big fan of
those WARN_ON but that's not a problem related to your patch, as you
said we are *not* handling the ret value in the current mainline).

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> +
>  	/* Set up MSI IRQ domain */
>  	pci_set_msi_domain(dev);
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index abd5d5e17aee..1524adbb30ab 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -485,6 +485,7 @@ struct pci_host_bridge {
>  			resource_size_t start,
>  			resource_size_t size,
>  			resource_size_t align);
> +	int (*add_device)(struct pci_dev *dev);
>  	unsigned long	private[0] ____cacheline_aligned;
>  };
>  
> -- 
> 2.18.0
>
Christoph Hellwig Aug. 6, 2018, 12:30 p.m. | #2
On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> >  	pci_configure_device(dev);
> > @@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> >  	ret = pcibios_add_device(dev);
> >  	WARN_ON(ret < 0);
> >  
> > +	if (host->add_device) {
> > +		ret = host->add_device(dev);
> > +		WARN_ON(ret < 0);
> > +	}
> 
> This looks fine; we could go a step further and make the hunk above
> the default (weak) implementation of pcibios_add_device() that is
> currently a NOP returning 0, I will remove it for v4.20.

I'd love to see pcibios_add_device go away entirely.  But I wonder how to
get setup the pci_host_bridge pointer for the remaining architectures that
still implement it.  I did look for any easy way but couldn't find one.
But then I don't really know this area of the PCI code too well.
Arnd Bergmann Aug. 6, 2018, 1:54 p.m. | #3
On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > >     pci_configure_device(dev);
> > > @@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > >     ret = pcibios_add_device(dev);
> > >     WARN_ON(ret < 0);
> > >
> > > +   if (host->add_device) {
> > > +           ret = host->add_device(dev);
> > > +           WARN_ON(ret < 0);
> > > +   }
> >
> > This looks fine; we could go a step further and make the hunk above
> > the default (weak) implementation of pcibios_add_device() that is
> > currently a NOP returning 0, I will remove it for v4.20.
>
> I'd love to see pcibios_add_device go away entirely.  But I wonder how to
> get setup the pci_host_bridge pointer for the remaining architectures that
> still implement it.  I did look for any easy way but couldn't find one.
> But then I don't really know this area of the PCI code too well.

If the platform doesn't already have a pointer to its pci_host_bridge
structure, it can call pci_find_host_bridge(bus->dev) to get it.

Ideally, platforms (either arch specific code or drivers/pci/controller)
should call pci_alloc_host_bridge()/pci_register_host_bridge()
instead of the traditional pci_create_root_bus() that is less flexible.
That way, the driver specific structure can get allocated together with
the host bridge.

         Arnd
Lorenzo Pieralisi Aug. 6, 2018, 2:55 p.m. | #4
On Mon, Aug 06, 2018 at 03:54:13PM +0200, Arnd Bergmann wrote:
> On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > > >     pci_configure_device(dev);
> > > > @@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > > >     ret = pcibios_add_device(dev);
> > > >     WARN_ON(ret < 0);
> > > >
> > > > +   if (host->add_device) {
> > > > +           ret = host->add_device(dev);
> > > > +           WARN_ON(ret < 0);
> > > > +   }
> > >
> > > This looks fine; we could go a step further and make the hunk above
> > > the default (weak) implementation of pcibios_add_device() that is
> > > currently a NOP returning 0, I will remove it for v4.20.
> >
> > I'd love to see pcibios_add_device go away entirely.  But I wonder how to
> > get setup the pci_host_bridge pointer for the remaining architectures that
> > still implement it.  I did look for any easy way but couldn't find one.
> > But then I don't really know this area of the PCI code too well.
> 
> If the platform doesn't already have a pointer to its pci_host_bridge
> structure, it can call pci_find_host_bridge(bus->dev) to get it.
> 
> Ideally, platforms (either arch specific code or drivers/pci/controller)
> should call pci_alloc_host_bridge()/pci_register_host_bridge()
> instead of the traditional pci_create_root_bus() that is less flexible.
> That way, the driver specific structure can get allocated together with
> the host bridge.

Yes that's basically the gist, it should be a matter of identifying code
that creates the root bus (explicitly or implicitly - ie
pci_scan_root_bus()) and patching it up, probably something to be done
(and moved into -next) early -rc* since it is something that can easily
break the world if we miss some code paths. I will have a look into
that and can take this patch for that series if it does not go into
v4.19.

Thanks,
Lorenzo
Arnd Bergmann Aug. 6, 2018, 7:49 p.m. | #5
On Mon, Aug 6, 2018 at 4:56 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Aug 06, 2018 at 03:54:13PM +0200, Arnd Bergmann wrote:
> > On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > > > >     pci_configure_device(dev);
> > > > > @@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > > > >     ret = pcibios_add_device(dev);
> > > > >     WARN_ON(ret < 0);
> > > > >
> > > > > +   if (host->add_device) {
> > > > > +           ret = host->add_device(dev);
> > > > > +           WARN_ON(ret < 0);
> > > > > +   }
> > > >
> > > > This looks fine; we could go a step further and make the hunk above
> > > > the default (weak) implementation of pcibios_add_device() that is
> > > > currently a NOP returning 0, I will remove it for v4.20.
> > >
> > > I'd love to see pcibios_add_device go away entirely.  But I wonder how to
> > > get setup the pci_host_bridge pointer for the remaining architectures that
> > > still implement it.  I did look for any easy way but couldn't find one.
> > > But then I don't really know this area of the PCI code too well.
> >
> > If the platform doesn't already have a pointer to its pci_host_bridge
> > structure, it can call pci_find_host_bridge(bus->dev) to get it.
> >
> > Ideally, platforms (either arch specific code or drivers/pci/controller)
> > should call pci_alloc_host_bridge()/pci_register_host_bridge()
> > instead of the traditional pci_create_root_bus() that is less flexible.
> > That way, the driver specific structure can get allocated together with
> > the host bridge.
>
> Yes that's basically the gist, it should be a matter of identifying code
> that creates the root bus (explicitly or implicitly - ie
> pci_scan_root_bus()) and patching it up, probably something to be done
> (and moved into -next) early -rc* since it is something that can easily
> break the world if we miss some code paths. I will have a look into
> that and can take this patch for that series if it does not go into
> v4.19.

I've started prototyping something now, will post something tomorrow.
I have an idea for a version that should be minimally invasive, at the
cost of duplicating some code in drivers that still use the old interfaces.

        Arnd

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac876e32de4b..452190fb05e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2290,6 +2290,7 @@  static void pci_set_msi_domain(struct pci_dev *dev)
 
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
 	int ret;
 
 	pci_configure_device(dev);
@@ -2328,6 +2329,11 @@  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	ret = pcibios_add_device(dev);
 	WARN_ON(ret < 0);
 
+	if (host->add_device) {
+		ret = host->add_device(dev);
+		WARN_ON(ret < 0);
+	}
+
 	/* Set up MSI IRQ domain */
 	pci_set_msi_domain(dev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index abd5d5e17aee..1524adbb30ab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -485,6 +485,7 @@  struct pci_host_bridge {
 			resource_size_t start,
 			resource_size_t size,
 			resource_size_t align);
+	int (*add_device)(struct pci_dev *dev);
 	unsigned long	private[0] ____cacheline_aligned;
 };