linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain to zero
@ 2021-04-12 12:39 Pali Rohár
  2021-04-13 18:17 ` Rob Herring
  0 siblings, 1 reply; 10+ messages in thread
From: Pali Rohár @ 2021-04-12 12:39 UTC (permalink / raw)
  To: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Rob Herring,
	Marek Behún
  Cc: linux-arm-kernel, devicetree, linux-kernel, linux-pci

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(+)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain to zero
  2021-04-12 12:39 [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain to zero Pali Rohár
@ 2021-04-13 18:17 ` Rob Herring
  2021-04-15  8:36   ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
  0 siblings, 1 reply; 10+ messages in thread
From: Rob Herring @ 2021-04-13 18:17 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
	Marek Behún, linux-arm-kernel, devicetree, linux-kernel,
	PCI

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
  2021-04-13 18:17 ` Rob Herring
@ 2021-04-15  8:36   ` Pali Rohár
  2021-04-15  8:45     ` Marek Behun
  0 siblings, 1 reply; 10+ messages in thread
From: Pali Rohár @ 2021-04-15  8:36 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
	Marek Behún, linux-arm-kernel, devicetree, linux-kernel,
	PCI

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
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
  2021-04-15  8:36   ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
@ 2021-04-15  8:45     ` Marek Behun
  2021-04-15 15:13       ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain " Rob Herring
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Behun @ 2021-04-15  8:45 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Rob Herring, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
	linux-arm-kernel, devicetree, linux-kernel, PCI

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain to zero
  2021-04-15  8:45     ` Marek Behun
@ 2021-04-15 15:13       ` Rob Herring
  2021-04-17 14:49         ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
  0 siblings, 1 reply; 10+ messages in thread
From: Rob Herring @ 2021-04-15 15:13 UTC (permalink / raw)
  To: Marek Behun, Pali Rohár
  Cc: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
	linux-arm-kernel, devicetree, linux-kernel, PCI

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
  2021-04-15 15:13       ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain " Rob Herring
@ 2021-04-17 14:49         ` Pali Rohár
  2021-04-17 15:19           ` Andrew Lunn
  2021-04-23 15:33           ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain " Rob Herring
  0 siblings, 2 replies; 10+ messages in thread
From: Pali Rohár @ 2021-04-17 14:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: Marek Behun, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
	linux-arm-kernel, devicetree, linux-kernel, PCI

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?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
  2021-04-17 14:49         ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
@ 2021-04-17 15:19           ` Andrew Lunn
  2021-04-17 19:42             ` Pali Rohár
  2021-04-23 15:33           ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain " Rob Herring
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2021-04-17 15:19 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Rob Herring, Marek Behun, Gregory Clement, Sebastian Hesselbarth,
	linux-arm-kernel, devicetree, linux-kernel, PCI

> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
  2021-04-17 15:19           ` Andrew Lunn
@ 2021-04-17 19:42             ` Pali Rohár
  0 siblings, 0 replies; 10+ messages in thread
From: Pali Rohár @ 2021-04-17 19:42 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Rob Herring, Marek Behun, Gregory Clement, Sebastian Hesselbarth,
	linux-arm-kernel, devicetree, linux-kernel, PCI

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).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain to zero
  2021-04-17 14:49         ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
  2021-04-17 15:19           ` Andrew Lunn
@ 2021-04-23 15:33           ` Rob Herring
  2021-04-25 15:21             ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
  1 sibling, 1 reply; 10+ messages in thread
From: Rob Herring @ 2021-04-23 15:33 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Marek Behun, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
	linux-arm-kernel, devicetree, linux-kernel, PCI

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
  2021-04-23 15:33           ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain " Rob Herring
@ 2021-04-25 15:21             ` Pali Rohár
  0 siblings, 0 replies; 10+ messages in thread
From: Pali Rohár @ 2021-04-25 15:21 UTC (permalink / raw)
  To: Rob Herring
  Cc: Marek Behun, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
	linux-arm-kernel, devicetree, linux-kernel, PCI

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-04-25 15:24 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 12:39 [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain to zero Pali Rohár
2021-04-13 18:17 ` Rob Herring
2021-04-15  8:36   ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
2021-04-15  8:45     ` Marek Behun
2021-04-15 15:13       ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain " Rob Herring
2021-04-17 14:49         ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár
2021-04-17 15:19           ` Andrew Lunn
2021-04-17 19:42             ` Pali Rohár
2021-04-23 15:33           ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux, pci-domain " Rob Herring
2021-04-25 15:21             ` [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain " Pali Rohár

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).