| Message ID | 20210412123936.25555-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: marvell: armada-37xx: Set linux, pci-domain to zero | expand |
On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote: > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' > function and allow to build it as module") PCIe controller driver for > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver > allows dynamic binding and unbinding of PCIe controller device. > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain > number (starting from zero) for this PCIe controller every time when device > is bound. So PCI domain changes after every unbind / bind operation. PCI host bridges as a module are relatively new, so seems likely a bug to me. > Alternative way for assigning PCI domain number is to use static allocated > numbers defined in Device Tree. This option has requirement that every PCI > controller in system must have defined PCI bus number in Device Tree. That seems entirely pointless from a DT point of view with a single PCI bridge. > Armada 37xx has only one PCIe controller, so assign for it PCI domain 0 in > Device Tree. > > After this change PCI domain on Armada 37xx is always zero, even after > repeated unbind and bind operations. > > Signed-off-by: Pali Rohár <pali@kernel.org> > Fixes: 526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module") > --- > arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi > index 7a2df148c6a3..f02058ef5364 100644 > --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi > +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi > @@ -495,6 +495,7 @@ > <0 0 0 2 &pcie_intc 1>, > <0 0 0 3 &pcie_intc 2>, > <0 0 0 4 &pcie_intc 3>; > + linux,pci-domain = <0>; > max-link-speed = <2>; > phys = <&comphy1 0>; > pcie_intc: interrupt-controller { > -- > 2.20.1 >
On Tuesday 13 April 2021 13:17:29 Rob Herring wrote: > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote: > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' > > function and allow to build it as module") PCIe controller driver for > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver > > allows dynamic binding and unbinding of PCIe controller device. > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain > > number (starting from zero) for this PCIe controller every time when device > > is bound. So PCI domain changes after every unbind / bind operation. > > PCI host bridges as a module are relatively new, so seems likely a bug to me. Why a bug? It is there since 5.10 and it is working. > > Alternative way for assigning PCI domain number is to use static allocated > > numbers defined in Device Tree. This option has requirement that every PCI > > controller in system must have defined PCI bus number in Device Tree. > > That seems entirely pointless from a DT point of view with a single PCI bridge. If domain id is not specified in DT then kernel uses counter and assigns counter++. So it is not pointless if we want to have stable domain id. > > Armada 37xx has only one PCIe controller, so assign for it PCI domain 0 in > > Device Tree. > > > > After this change PCI domain on Armada 37xx is always zero, even after > > repeated unbind and bind operations. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Fixes: 526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module") > > --- > > arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi > > index 7a2df148c6a3..f02058ef5364 100644 > > --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi > > +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi > > @@ -495,6 +495,7 @@ > > <0 0 0 2 &pcie_intc 1>, > > <0 0 0 3 &pcie_intc 2>, > > <0 0 0 4 &pcie_intc 3>; > > + linux,pci-domain = <0>; > > max-link-speed = <2>; > > phys = <&comphy1 0>; > > pcie_intc: interrupt-controller { > > -- > > 2.20.1 > >
On Thu, 15 Apr 2021 10:36:40 +0200 Pali Rohár <pali@kernel.org> wrote: > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote: > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' > > > function and allow to build it as module") PCIe controller driver for > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver > > > allows dynamic binding and unbinding of PCIe controller device. > > > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain > > > number (starting from zero) for this PCIe controller every time when device > > > is bound. So PCI domain changes after every unbind / bind operation. > > > > PCI host bridges as a module are relatively new, so seems likely a bug to me. > > Why a bug? It is there since 5.10 and it is working. > > > > Alternative way for assigning PCI domain number is to use static allocated > > > numbers defined in Device Tree. This option has requirement that every PCI > > > controller in system must have defined PCI bus number in Device Tree. > > > > That seems entirely pointless from a DT point of view with a single PCI bridge. > > If domain id is not specified in DT then kernel uses counter and assigns > counter++. So it is not pointless if we want to have stable domain id. What Rob is trying to say is that - the bug is that kernel assigns counter++ - device-tree should not be used to fix problems with how kernel does things - if a device has only one PCIe controller, it is pointless to define it's pci-domain. If there were multiple controllers, then it would make sense, but there is only one
On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote: > > On Thu, 15 Apr 2021 10:36:40 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote: > > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' > > > > function and allow to build it as module") PCIe controller driver for > > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver > > > > allows dynamic binding and unbinding of PCIe controller device. > > > > > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain > > > > number (starting from zero) for this PCIe controller every time when device > > > > is bound. So PCI domain changes after every unbind / bind operation. > > > > > > PCI host bridges as a module are relatively new, so seems likely a bug to me. > > > > Why a bug? It is there since 5.10 and it is working. I mean historically, the PCI subsystem didn't even support host bridges as a module. They weren't even proper drivers and it was all arch specific code. Most of the host bridge drivers are still built-in only. This seems like a small detail that was easily overlooked. unbind is not a well tested path. > > > > Alternative way for assigning PCI domain number is to use static allocated > > > > numbers defined in Device Tree. This option has requirement that every PCI > > > > controller in system must have defined PCI bus number in Device Tree. > > > > > > That seems entirely pointless from a DT point of view with a single PCI bridge. > > > > If domain id is not specified in DT then kernel uses counter and assigns > > counter++. So it is not pointless if we want to have stable domain id. > > What Rob is trying to say is that > - the bug is that kernel assigns counter++ > - device-tree should not be used to fix problems with how kernel does > things > - if a device has only one PCIe controller, it is pointless to define > it's pci-domain. If there were multiple controllers, then it would > make sense, but there is only one Yes. I think what we want here is a domain bitmap rather than a counter and we assign the lowest free bit. That could also allow for handling a mixture of fixed domain numbers and dynamically assigned ones. You could create scenarios where the numbers change on you, but it wouldn't be any different than say plugging in USB serial adapters. You get the same ttyUSBx device when you re-attach unless there's been other ttyUSBx devices attached/detached. Rob
On Thursday 15 April 2021 10:13:17 Rob Herring wrote: > On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Thu, 15 Apr 2021 10:36:40 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote: > > > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' > > > > > function and allow to build it as module") PCIe controller driver for > > > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver > > > > > allows dynamic binding and unbinding of PCIe controller device. > > > > > > > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain > > > > > number (starting from zero) for this PCIe controller every time when device > > > > > is bound. So PCI domain changes after every unbind / bind operation. > > > > > > > > PCI host bridges as a module are relatively new, so seems likely a bug to me. > > > > > > Why a bug? It is there since 5.10 and it is working. > > I mean historically, the PCI subsystem didn't even support host > bridges as a module. They weren't even proper drivers and it was all > arch specific code. Most of the host bridge drivers are still built-in > only. This seems like a small detail that was easily overlooked. > unbind is not a well tested path. Ok! Just to note that during my testing I have not spotted any issue. > > > > > Alternative way for assigning PCI domain number is to use static allocated > > > > > numbers defined in Device Tree. This option has requirement that every PCI > > > > > controller in system must have defined PCI bus number in Device Tree. > > > > > > > > That seems entirely pointless from a DT point of view with a single PCI bridge. > > > > > > If domain id is not specified in DT then kernel uses counter and assigns > > > counter++. So it is not pointless if we want to have stable domain id. > > > > What Rob is trying to say is that > > - the bug is that kernel assigns counter++ > > - device-tree should not be used to fix problems with how kernel does > > things > > - if a device has only one PCIe controller, it is pointless to define > > it's pci-domain. If there were multiple controllers, then it would > > make sense, but there is only one > > Yes. I think what we want here is a domain bitmap rather than a > counter and we assign the lowest free bit. That could also allow for > handling a mixture of fixed domain numbers and dynamically assigned > ones. Currently this code is implemented in pci_bus_find_domain_nr() function. IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB of memory. I'm not sure if it is fine or some other tree-based structure for allocated domain numbers is needed. > You could create scenarios where the numbers change on you, but it > wouldn't be any different than say plugging in USB serial adapters. > You get the same ttyUSBx device when you re-attach unless there's been > other ttyUSBx devices attached/detached. This should be fine for most scenarios. Dynamically attaching / detaching PCI domain is not such common action... Will you implement this new feature?
> Currently this code is implemented in pci_bus_find_domain_nr() function. > IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB > of memory. I'm not sure if it is fine or some other tree-based structure > for allocated domain numbers is needed. Hi Pali Have a look at lib/idr.c Andrew
On Saturday 17 April 2021 17:19:38 Andrew Lunn wrote: > > Currently this code is implemented in pci_bus_find_domain_nr() function. > > IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB > > of memory. I'm not sure if it is fine or some other tree-based structure > > for allocated domain numbers is needed. > > Hi Pali > > Have a look at lib/idr.c > > Andrew Great! So number allocation is already implemented in kernel (via radix trees).
On Sat, Apr 17, 2021 at 9:49 AM Pali Rohár <pali@kernel.org> wrote: > > On Thursday 15 April 2021 10:13:17 Rob Herring wrote: > > On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > On Thu, 15 Apr 2021 10:36:40 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote: > > > > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' > > > > > > function and allow to build it as module") PCIe controller driver for > > > > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver > > > > > > allows dynamic binding and unbinding of PCIe controller device. > > > > > > > > > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain > > > > > > number (starting from zero) for this PCIe controller every time when device > > > > > > is bound. So PCI domain changes after every unbind / bind operation. > > > > > > > > > > PCI host bridges as a module are relatively new, so seems likely a bug to me. > > > > > > > > Why a bug? It is there since 5.10 and it is working. > > > > I mean historically, the PCI subsystem didn't even support host > > bridges as a module. They weren't even proper drivers and it was all > > arch specific code. Most of the host bridge drivers are still built-in > > only. This seems like a small detail that was easily overlooked. > > unbind is not a well tested path. > > Ok! Just to note that during my testing I have not spotted any issue. > > > > > > > Alternative way for assigning PCI domain number is to use static allocated > > > > > > numbers defined in Device Tree. This option has requirement that every PCI > > > > > > controller in system must have defined PCI bus number in Device Tree. > > > > > > > > > > That seems entirely pointless from a DT point of view with a single PCI bridge. > > > > > > > > If domain id is not specified in DT then kernel uses counter and assigns > > > > counter++. So it is not pointless if we want to have stable domain id. > > > > > > What Rob is trying to say is that > > > - the bug is that kernel assigns counter++ > > > - device-tree should not be used to fix problems with how kernel does > > > things > > > - if a device has only one PCIe controller, it is pointless to define > > > it's pci-domain. If there were multiple controllers, then it would > > > make sense, but there is only one > > > > Yes. I think what we want here is a domain bitmap rather than a > > counter and we assign the lowest free bit. That could also allow for > > handling a mixture of fixed domain numbers and dynamically assigned > > ones. > > Currently this code is implemented in pci_bus_find_domain_nr() function. > IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB > of memory. I'm not sure if it is fine or some other tree-based structure > for allocated domain numbers is needed. It's an atomic_t but then shortened (potentially) to an 'int'. Surely we don't need 8k (or 2^31) host bridges? Seems like we could start with 64 and bump it as needed. Or the idr route is another option if that works. We'd need to get the lowest free value and be able to reserve values (when specified by firmware). > > You could create scenarios where the numbers change on you, but it > > wouldn't be any different than say plugging in USB serial adapters. > > You get the same ttyUSBx device when you re-attach unless there's been > > other ttyUSBx devices attached/detached. > > This should be fine for most scenarios. Dynamically attaching / > detaching PCI domain is not such common action... > > Will you implement this new feature? Yes, after I find a DT binding co-maintainer. Rob
On Friday 23 April 2021 10:33:38 Rob Herring wrote: > On Sat, Apr 17, 2021 at 9:49 AM Pali Rohár <pali@kernel.org> wrote: > > > > On Thursday 15 April 2021 10:13:17 Rob Herring wrote: > > > On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > On Thu, 15 Apr 2021 10:36:40 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote: > > > > > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' > > > > > > > function and allow to build it as module") PCIe controller driver for > > > > > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver > > > > > > > allows dynamic binding and unbinding of PCIe controller device. > > > > > > > > > > > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain > > > > > > > number (starting from zero) for this PCIe controller every time when device > > > > > > > is bound. So PCI domain changes after every unbind / bind operation. > > > > > > > > > > > > PCI host bridges as a module are relatively new, so seems likely a bug to me. > > > > > > > > > > Why a bug? It is there since 5.10 and it is working. > > > > > > I mean historically, the PCI subsystem didn't even support host > > > bridges as a module. They weren't even proper drivers and it was all > > > arch specific code. Most of the host bridge drivers are still built-in > > > only. This seems like a small detail that was easily overlooked. > > > unbind is not a well tested path. > > > > Ok! Just to note that during my testing I have not spotted any issue. > > > > > > > > > Alternative way for assigning PCI domain number is to use static allocated > > > > > > > numbers defined in Device Tree. This option has requirement that every PCI > > > > > > > controller in system must have defined PCI bus number in Device Tree. > > > > > > > > > > > > That seems entirely pointless from a DT point of view with a single PCI bridge. > > > > > > > > > > If domain id is not specified in DT then kernel uses counter and assigns > > > > > counter++. So it is not pointless if we want to have stable domain id. > > > > > > > > What Rob is trying to say is that > > > > - the bug is that kernel assigns counter++ > > > > - device-tree should not be used to fix problems with how kernel does > > > > things > > > > - if a device has only one PCIe controller, it is pointless to define > > > > it's pci-domain. If there were multiple controllers, then it would > > > > make sense, but there is only one > > > > > > Yes. I think what we want here is a domain bitmap rather than a > > > counter and we assign the lowest free bit. That could also allow for > > > handling a mixture of fixed domain numbers and dynamically assigned > > > ones. > > > > Currently this code is implemented in pci_bus_find_domain_nr() function. > > IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB > > of memory. I'm not sure if it is fine or some other tree-based structure > > for allocated domain numbers is needed. > > It's an atomic_t but then shortened (potentially) to an 'int'. Surely > we don't need 8k (or 2^31) host bridges? Seems like we could start > with 64 and bump it as needed. Or the idr route is another option if > that works. We'd need to get the lowest free value and be able to > reserve values (when specified by firmware). Seems that idr_alloc() supports both required operations as you can ask idr_alloc() for allocating specific id (if is available). > > > You could create scenarios where the numbers change on you, but it > > > wouldn't be any different than say plugging in USB serial adapters. > > > You get the same ttyUSBx device when you re-attach unless there's been > > > other ttyUSBx devices attached/detached. > > > > This should be fine for most scenarios. Dynamically attaching / > > detaching PCI domain is not such common action... > > > > Will you implement this new feature? > > Yes, after I find a DT binding co-maintainer. > > Rob
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi index 7a2df148c6a3..f02058ef5364 100644 --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi @@ -495,6 +495,7 @@ <0 0 0 2 &pcie_intc 1>, <0 0 0 3 &pcie_intc 2>, <0 0 0 4 &pcie_intc 3>; + linux,pci-domain = <0>; max-link-speed = <2>; phys = <&comphy1 0>; pcie_intc: interrupt-controller {
Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module") PCIe controller driver for Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver allows dynamic binding and unbinding of PCIe controller device. Kernel PCI subsystem assigns by default dynamically allocated PCI domain number (starting from zero) for this PCIe controller every time when device is bound. So PCI domain changes after every unbind / bind operation. Alternative way for assigning PCI domain number is to use static allocated numbers defined in Device Tree. This option has requirement that every PCI controller in system must have defined PCI bus number in Device Tree. Armada 37xx has only one PCIe controller, so assign for it PCI domain 0 in Device Tree. After this change PCI domain on Armada 37xx is always zero, even after repeated unbind and bind operations. Signed-off-by: Pali Rohár <pali@kernel.org> Fixes: 526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module") --- arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 + 1 file changed, 1 insertion(+)