Date:   Wed, 18 Dec 2019 00:54:25 +0000
From:   Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Logan Gunthorpe <logang@...tatee.com>
Subject: Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt
 enumeration

On Tue, Dec 17, 2019 at 09:12:48AM -0600, Bjorn Helgaas wrote:
> On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> > Hi all,
> > 
> > Since last time:
> > 	Reverse Christmas tree for a couple of variables
> > 
> > 	Changed while to whilst (sounds more formal, and so that 
> > 	grepping for "while" only brings up code)
> > 
> > 	Made sure they still apply to latest Linux v5.5-rc1
> > 
> > Kind regards,
> > Nicholas
> > 
> > Nicholas Johnson (4):
> >   PCI: Consider alignment of hot-added bridges when distributing
> >     available resources
> >   PCI: In extend_bridge_window() change available to new_size
> >   PCI: Change extend_bridge_window() to set resource size directly
> >   PCI: Allow extend_bridge_window() to shrink resource if necessary
> 
> I have tentatively applied these to pci/resource for v5.6, but I need
> some kind of high-level description for why we want these changes.
I could not find these in linux-next (whereas it was almost immediate 
last time), so this must be why.

> 
> The commit logs describe what the code does, and that's good, but we
> really need a little more of the *why* and what the user-visible
> benefit is.  I know some of this was in earlier postings, but it seems
> to have gotten lost along the way.
Is this explanation going into the commit notes, or is this just to get 
it past reviewers, Greg K-H and Linus Torvalds?

Here is an attempt:

Although Thunderbolt 3 does not need special treatment like Cardbus, it 
does require the kernel to be up to date with PCI hotplug and capable of 
assigning resources when native enumeration mode is used (as opposed to 
BIOS-assisted mode). Operating systems have neglected this over the 
years, as PCI hotplug is not widely used.

Thunderbolt has a structure where each downstream facing port is a PCI 
hotplug bridge. Peripheral devices may have a second Thunderbolt port 
for daisy chaining more devices, much like Firewire did.

In this example, the controller is at bus 03, and this controller has 
two ports. Bus 05 is the NHI (Native Host Interface) and Bus 39 is the 
USB controller. Bus 07 is a Thunderbolt dock, as is Bus 0d, daisy 
chained after the first dock. The hotplug bridges are 04.01, 04.04, 
07.04, 0d.04, and depending on the implementation, 1c.4 under the host 
controller.

+-1c.4-[03-6c]----00.0-[04-6c]--+-00.0-[05]----00.0
|                               +-01.0-[06-38]----00.0-[07-38]--+-00.0-[08]----00.0
|                               |                               +-01.0-[09]----00.0
|                               |                               +-02.0-[0a]--
|                               |                               +-03.0-[0b]--
|                               |                               \-04.0-[0c-38]----00.0-[0d-38]--+-00.0-[0e]----00.0
|                               |                                                               +-01.0-[0f]----00.0
|                               |                                                               +-02.0-[10]--
|                               |                                                               +-03.0-[11]--
|                               |                                                               \-04.0-[12-38]--
|                               +-02.0-[39]----00.0
|                               \-04.0-[3a-6c]--

The host controller implementation appears differently in new Intel Ice 
Lake systems, with the Thunderbolt controllers integrated into the CPU, 
with the Thunderbolt ports appearing as chipset root ports.

Ice Lake with 2/4 Thunderbolt 3 ports implemented (0d.0 is the USB 
controller and 0d.2 is NHI #0):

-[0000:00]-+-00.0
           +-02.0
           +-04.0
           +-07.0-[01-2b]--
           +-07.1-[2c-56]--
           +-0d.0
           +-0d.2

Thunderbolt is unusual in that we have nested hotplug bridges. Mika 
Westerberg <mika.westerberg@...ux.intel.com> has done most of the work 
required to assign all unused resources (busn and mem) from the parent 
bridge window to the downstream hotplug bridge, allowing for more 
Thunderbolt devices to be daisy-chained. However, in 
drivers/pci/setup-bus.c in pci_bus_distribute_available_resources(), I 
noticed problems with hot-adding Thunderbolt devices containing PCI 
devices with resource alignment above the minimum 1M. Furthermore, I 
found it more reliable to cease the use of additional size resource 
lists, which are considered "optional" when allocating. The operation of 
pci_bus_distribute_available_resources() is only as guaranteed as the 
resource assignment. None of this would work with resources assigned by 
the kernel parameters 
pci=hpmmiosize=nn[KMG],hpmmiosize=nn[KMG],hpmmioprefsize=nn[KMG] - it 
would only work if the resources under the root port were assigned by 
firmware.

I have solved the alignment problem with patch 1/4 in 
pci_bus_distribute_available_resources() and the remaining issues with 
patches 2-4 by changing extend_bridge_window() which is now named 
adjust_bridge_window(). You can find the link to the bug report filed by 
Mika Westerberg in patch 1/4 commit log.

To sum up, although this could be applicable to any hypothetical 
application with nested PCI hotplug bridges, the intended end user 
facing case is Thunderbolt 3 with native enumeration mode. My changes:
- Allow for hot-adding PCI devices with >1M alignment of BARs
- Offer improved resilience and reliability due to removal of add_size
- Allow this to work with resources assigned with the kernel parameters
  pci=hpmmiosize=nn[KMG],hpmmiosize=nn[KMG],hpmmioprefsize=nn[KMG] as 
  opposed to only firmware-allocated resources.



Please let me know if this is not adequate for the required purposes.

Thanks!

Kind regards,
Nicholas Johnson

> 
> Bjorn

Powered by blists - more mailing lists