| Message ID | 20180804101402.10022-2-hch@lst.de |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [1/3] PCI: add a callback to struct pci_host_bridge for adding a new device | expand |
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 >
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.
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
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
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
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; };
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(+)