| Message ID | 20180801151403.20660-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/3] PCI: add a callback to struct pci_host_bridge for adding a new device | expand |
On Wed, Aug 01, 2018 at 05:14:01PM +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_dev" 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 | 4 ++++ > include/linux/pci.h | 1 + > 2 files changed, 5 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index ac876e32de4b..8736d78ffc66 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); > @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) > /* Set up MSI IRQ domain */ > pci_set_msi_domain(dev); > > + if (host->add_dev) > + host->add_dev(dev); Hi Christoph, while at it, IMHO it would be good to match the current pcibios_add_device() prototype (ie returning an int error value) so that this change can pave the way to removing yet another pcibios call. Actually we could make pcibios_add_device() removal as part of this series but you would have to patch all related host bridge drivers initializations so I am happy for this patch to go standalone (I can patch the rest of the code) - I leave it to Bjorn's decision. Thanks, Lorenzo > + > /* Notifier could use PCI capabilities */ > dev->match_driver = false; > ret = device_add(&dev->dev); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index abd5d5e17aee..5eedae8e8f2b 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); > + void (*add_dev)(struct pci_dev *dev); > unsigned long private[0] ____cacheline_aligned; > }; > > -- > 2.18.0 >
On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote: > while at it, IMHO it would be good to match the current > pcibios_add_device() prototype (ie returning an int error value) > so that this change can pave the way to removing yet another > pcibios call. Replacing pcibios_add_device sounds like a good idea. But then again we can't actually handle the error of that one either right now..
On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote: > On Wed, Aug 01, 2018 at 05:14:01PM +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_dev" 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 | 4 ++++ > > include/linux/pci.h | 1 + > > 2 files changed, 5 insertions(+) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index ac876e32de4b..8736d78ffc66 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); > > @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) > > /* Set up MSI IRQ domain */ > > pci_set_msi_domain(dev); > > > > + if (host->add_dev) > > + host->add_dev(dev); > > Hi Christoph, > > while at it, IMHO it would be good to match the current > pcibios_add_device() prototype (ie returning an int error value) > so that this change can pave the way to removing yet another > pcibios call. > > Actually we could make pcibios_add_device() removal as part of this > series but you would have to patch all related host bridge drivers > initializations so I am happy for this patch to go standalone (I can > patch the rest of the code) - I leave it to Bjorn's decision. This patch seems OK to me. I don't really care about the prototype. There's only one pcibios_add_device() implementation (x86) that returns anything other than 0, and that's a pretty obscure error case related to f9a37be0f02a ("x86: Use PCI setup data"), which lets us use ROM data from boot services. Even then the only thing that happens is a WARN_ON(). A more descriptive printk would be a lot more useful. I assume you'll probably merge this along with the rest of the series, Lorenzo, so: Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > + > > /* Notifier could use PCI capabilities */ > > dev->match_driver = false; > > ret = device_add(&dev->dev); > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index abd5d5e17aee..5eedae8e8f2b 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); > > + void (*add_dev)(struct pci_dev *dev); > > unsigned long private[0] ____cacheline_aligned; > > }; > > > > -- > > 2.18.0 > >
On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote: > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Aug 01, 2018 at 05:14:01PM +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_dev" 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 | 4 ++++ > > > include/linux/pci.h | 1 + > > > 2 files changed, 5 insertions(+) > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > index ac876e32de4b..8736d78ffc66 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); > > > @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) > > > /* Set up MSI IRQ domain */ > > > pci_set_msi_domain(dev); > > > > > > + if (host->add_dev) > > > + host->add_dev(dev); > > > > Hi Christoph, > > > > while at it, IMHO it would be good to match the current > > pcibios_add_device() prototype (ie returning an int error value) > > so that this change can pave the way to removing yet another > > pcibios call. > > > > Actually we could make pcibios_add_device() removal as part of this > > series but you would have to patch all related host bridge drivers > > initializations so I am happy for this patch to go standalone (I can > > patch the rest of the code) - I leave it to Bjorn's decision. > > This patch seems OK to me. > > I don't really care about the prototype. There's only one > pcibios_add_device() implementation (x86) that returns anything other > than 0, and that's a pretty obscure error case related to f9a37be0f02a > ("x86: Use PCI setup data"), which lets us use ROM data from boot > services. Even then the only thing that happens is a WARN_ON(). A > more descriptive printk would be a lot more useful. Thinking about this some more, I'm not so sure about the connection with removing pcibios_add_device(). This host_bridge->add_dev() hook would be for host bridge-specific things, while pcibios_add_device() is for arch-specific things. I'd still love to get rid of pcibios_add_device() (especially the non-arch-specific things like the pci_claim_resource() in s390); I'm just not sure yet whether this particular patch is the vehicle.
On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote: > > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote: > > This patch seems OK to me. > > > > I don't really care about the prototype. There's only one > > pcibios_add_device() implementation (x86) that returns anything other > > than 0, and that's a pretty obscure error case related to f9a37be0f02a > > ("x86: Use PCI setup data"), which lets us use ROM data from boot > > services. Even then the only thing that happens is a WARN_ON(). A > > more descriptive printk would be a lot more useful. > > Thinking about this some more, I'm not so sure about the connection > with removing pcibios_add_device(). This host_bridge->add_dev() hook > would be for host bridge-specific things, while pcibios_add_device() > is for arch-specific things. > > I'd still love to get rid of pcibios_add_device() (especially the > non-arch-specific things like the pci_claim_resource() in s390); I'm > just not sure yet whether this particular patch is the vehicle. I think most of the arch-specific pcibios_* calls are actually host bridge specific after all, it just so happens that they are implemented on architectures that only have one specific host bridge implementation, or that they are used on an architecture that does something odd in one place and needs to do something else in another place. For pci_claim_resource() we seem to be doing this in a number of different places, but there isn't strictly a reason for that. I've started a patch series to turn all the __weak pcibios_* functions into callbacks, which seems to be the right solution for almost all of them. Arnd
On Thu, Aug 16, 2018 at 11:04:53PM +0200, Arnd Bergmann wrote: > On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote: > > > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote: > > > > This patch seems OK to me. > > > > > > I don't really care about the prototype. There's only one > > > pcibios_add_device() implementation (x86) that returns anything other > > > than 0, and that's a pretty obscure error case related to f9a37be0f02a > > > ("x86: Use PCI setup data"), which lets us use ROM data from boot > > > services. Even then the only thing that happens is a WARN_ON(). A > > > more descriptive printk would be a lot more useful. > > > > Thinking about this some more, I'm not so sure about the connection > > with removing pcibios_add_device(). This host_bridge->add_dev() hook > > would be for host bridge-specific things, while pcibios_add_device() > > is for arch-specific things. > > > > I'd still love to get rid of pcibios_add_device() (especially the > > non-arch-specific things like the pci_claim_resource() in s390); I'm > > just not sure yet whether this particular patch is the vehicle. > > I think most of the arch-specific pcibios_* calls are actually > host bridge specific after all, it just so happens that they are > implemented on architectures that only have one specific > host bridge implementation, or that they are used on an > architecture that does something odd in one place and needs > to do something else in another place. > > For pci_claim_resource() we seem to be doing this in a number > of different places, but there isn't strictly a reason for that. pci_claim_resource() is needed if either arch code or the host controller driver does not trigger a resources assignment (which claims them while at it); in theory that's arch agnostic but it turned out to be very arch/platform specific - aka if we move s390 code to core code we will notice :) so pci_claim_resource() in a pcibios call is unfortunately legitimate - whether it can be moved out of it to generic code that's a very complicated problem. Lorenzo
On Fri, Aug 17, 2018 at 5:25 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Thu, Aug 16, 2018 at 11:04:53PM +0200, Arnd Bergmann wrote: > > On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote: > > > > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote: > > > > > > This patch seems OK to me. > > > > > > > > I don't really care about the prototype. There's only one > > > > pcibios_add_device() implementation (x86) that returns anything other > > > > than 0, and that's a pretty obscure error case related to f9a37be0f02a > > > > ("x86: Use PCI setup data"), which lets us use ROM data from boot > > > > services. Even then the only thing that happens is a WARN_ON(). A > > > > more descriptive printk would be a lot more useful. > > > > > > Thinking about this some more, I'm not so sure about the connection > > > with removing pcibios_add_device(). This host_bridge->add_dev() hook > > > would be for host bridge-specific things, while pcibios_add_device() > > > is for arch-specific things. > > > > > > I'd still love to get rid of pcibios_add_device() (especially the > > > non-arch-specific things like the pci_claim_resource() in s390); I'm > > > just not sure yet whether this particular patch is the vehicle. > > > > I think most of the arch-specific pcibios_* calls are actually > > host bridge specific after all, it just so happens that they are > > implemented on architectures that only have one specific > > host bridge implementation, or that they are used on an > > architecture that does something odd in one place and needs > > to do something else in another place. > > > > For pci_claim_resource() we seem to be doing this in a number > > of different places, but there isn't strictly a reason for that. > > pci_claim_resource() is needed if either arch code or the host > controller driver does not trigger a resources assignment (which claims > them while at it); in theory that's arch agnostic but it turned out to > be very arch/platform specific - aka if we move s390 code to core code > we will notice :) so pci_claim_resource() in a pcibios call is > unfortunately legitimate - whether it can be moved out of it to > generic code that's a very complicated problem. The generic pci_host_probe() already does either pci_bus_claim_resources() or pci_bus_size_bridges()/pci_bus_assign_resources()/pcie_bus_configure_settings() I would hope that we can move a lot of implementations over to just call this function, with the appropriate PCI_PROBE_ONLY setting. Arnd
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ac876e32de4b..8736d78ffc66 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); @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) /* Set up MSI IRQ domain */ pci_set_msi_domain(dev); + if (host->add_dev) + host->add_dev(dev); + /* Notifier could use PCI capabilities */ dev->match_driver = false; ret = device_add(&dev->dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index abd5d5e17aee..5eedae8e8f2b 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); + void (*add_dev)(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_dev" 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 | 4 ++++ include/linux/pci.h | 1 + 2 files changed, 5 insertions(+)