Re: [PATCH] ACPI / hotplug / PCI: Allocate resources directly under the non-hotplug bridge

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Nov 07, 2019 at 11:03:33AM +0200, Mika Westerberg wrote:
> On Wed, Nov 06, 2019 at 05:24:05PM -0600, Bjorn Helgaas wrote:
> > On Wed, Oct 30, 2019 at 06:05:45PM +0300, Mika Westerberg wrote:
> > > Valerio and others reported that commit 84c8b58ed3ad ("ACPI / hotplug /
> > > PCI: Don't scan bridges managed by native hotplug") prevents some recent
> > > LG and HP laptops from booting with endless loop of:
> > > 
> > >   [   26.237796] ACPI Error: No handler or method for GPE 08, disabling event (20190215/evgpe-835)
> > >   [   26.238699] ACPI Error: No handler or method for GPE 09, disabling event (20190215/evgpe-835)
> > >   [   26.239306] ACPI Error: No handler or method for GPE 0A, disabling event (20190215/evgpe-835)
> > >   ...
> > > 
> > > What seems to happen is that during boot, after the initial PCI
> > > enumeration when EC is enabled the platform triggers ACPI Notify() to
> > > one of the root ports. The root port itself looks like this:
> > > 
> > >   [    0.723757] pci 0000:00:1b.0: PCI bridge to [bus 02-3a]
> > >   [    0.723765] pci 0000:00:1b.0:   bridge window [mem 0xc4000000-0xda0fffff]
> > >   [    0.723773] pci 0000:00:1b.0:   bridge window [mem 0x80000000-0xa1ffffff 64bit pref]
> > >
> > > The BIOS has configured the root port so that it does not have I/O
> > > bridge window.
> > > 
> > > Now when the ACPI Notify() is triggered ACPI hotplug handler calls
> > > acpiphp_native_scan_bridge() for each non-hotplug bridge (as this system
> > > is using native PCIe hotplug) and pci_assign_unassigned_bridge_resources()
> > > to allocate resources.
> > > 
> > > The device connected to the root port is a PCIe switch (Thunderbolt
> > > controller) with two hotplug downstream ports. Because of the hotplug
> > > ports __pci_bus_size_bridges() tries to add "additional I/O" of 256
> > > bytes to each (DEFAULT_HOTPLUG_IO_SIZE). This gets further aligned to 4k
> > > as that's the minimum I/O window size so each hotplug port gets 4k I/O
> > > window and the same happens for the root port (which is also hotplug
> > > port). This means 3 * 4k = 12k I/O window.
> > > 
> > > Because of this pci_assign_unassigned_bridge_resources() ends up opening
> > > a I/O bridge window for the root port at first available I/O address
> > > which seems to be in range 0x1000 - 0x3fff. Normally this range is used
> > > for ACPI stuff such as GPE bits (below is part of /proc/ioports):
> > > 
> > >     1800-1803 : ACPI PM1a_EVT_BLK
> > >     1804-1805 : ACPI PM1a_CNT_BLK
> > >     1808-180b : ACPI PM_TMR
> > >     1810-1815 : ACPI CPU throttle
> > >     1850-1850 : ACPI PM2_CNT_BLK
> > >     1854-1857 : pnp 00:05
> > >     1860-187f : ACPI GPE0_BLK
> > 
> > Nice debugging work!
> > 
> > > However, when the ACPI Notify() happened this range was not yet reserved
> > > for ACPI/PNP (that happens later) so PCI gets it. 
> > 
> > I think this is really the underlying problem.  IMO those ACPI/PNP
> > resources ought to be reserved before we start assigning resources to
> > PCI devices.  That would be a huge problem to fix, though.
> 
> I agree, changing the order would probably open another can of worms.

Just for the record, I think it's even worse than just reserving them
in the wrong order.  IIRC, we don't reserve ACPI/PNP resources *at
all* except for PNP0C01 and PNP0C02 (drivers/pnp/system.c) and
whatever individual drivers reserve.  This is like pretending that
devices don't respond to their address space until a driver claims
them.  But I'm not suggesting opening this can of worms for *this*
problem.

> > > It then starts writing
> > > to this range and accidentally stomps over GPE bits among other things
> > > causing the endless stream of messages about missing GPE handler.
> > >
> > > This problem does not happen if "pci=hpiosize=0" is passed in the kernel
> > > command line. The reason is that then the kernel does not try to
> > > allocate the additional 256 bytes for each hotplug port.
> > > 
> > > Fix this by allocating resources directly below the non-hotplug bridges
> > > where a new device may appear as a result of ACPI Notify(). This avoids
> > > the hotplug bridges and prevents opening the additional I/O window.
> > 
> > Looking at [1], here's the topology I see:
> > 
> >   00:1b.0: Root Port to [bus 02-3a]                   # pciehp
> >   02:00.0: Switch Upstream Port to [bus 03-3a]        # thunderbolt
> >   03:00.0: Switch Downstream Port to [bus 04]         # thunderbolt
> >   03:01.0: Switch Downstream Port to [bus 05-39]      # thunderbolt, pciehp
> >   03:02.0: Switch Downstream Port to [bus 3a]         # thunderbolt
> >   04:00.0: reg 0x10: [mem 0xda000000-0xda03ffff]
> >   04:00.0: reg 0x14: [mem 0xda040000-0xda040fff]
> >   3a:00.0: reg 0x10: [mem 0xd9f00000-0xd9f0ffff]
> > 
> > In this topology, I guess the non-hotplug bridges would be 03:00.0
> > and 03:02.0?  And we skip 03:01.0 because hotplug_is_native() is true
> > for that?
> 
> Yes.
> 
> > What would happen if a device below one of the non-hotplug bridges,
> > e.g., 3a:00.0, had an I/O BAR?  Would this patch still work?
> 
> I think it would still work because now we call pci_bus_size_bridges()
> only for non-hotplug bridge which do not have I/O window open so
> pbus_size_io() fails to find the "free" I/O resource on that bus and the
> kernel then fails to assign that I/O resource for the device.

Not sure I understand; are you saying that we wouldn't have the EC/GPE
issue, but we'd be unable to use a device below 3a:00.0 that happened
to have an I/O BAR?  That doesn't sound optimal because there is I/O
space available that could be routed to 3a:00.0

Bjorn