[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-kernel
Subject:    Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
From:       Rob Herring <robh+dt () kernel ! org>
Date:       2021-04-23 15:33:38
Message-ID: CAL_Jsq+fNGQ+qMuqQ_c1Nou1h4R=-R5sn2n0p7rx==+e2JybSg () mail ! gmail ! com
[Download RAW message or body]

On Sat, Apr 17, 2021 at 9:49 AM Pali Roh=C3=A1r <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=C3=A1r <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=C3=A1r <pali@kernel.org>=
 wrote:
> > > > > >
> > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 're=
move'
> > > > > > function and allow to build it as module") PCIe controller driv=
er 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 P=
CI domain
> > > > > > number (starting from zero) for this PCIe controller every time=
 when device
> > > > > > is bound. So PCI domain changes after every unbind / bind opera=
tion.
> > > > >
> > > > > 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 stati=
c allocated
> > > > > > numbers defined in Device Tree. This option has requirement tha=
t 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 sing=
le PCI bridge.
> > > >
> > > > If domain id is not specified in DT then kernel uses counter and as=
signs
> > > > 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
[prev in list] [next in list] [prev in thread] [next in thread]