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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[+cc Ben]

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.

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

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?

[1] https://bugzilla.kernel.org/show_bug.cgi?id=203617#c49

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203617
> Fixes: 84c8b58ed3ad ("ACPI / hotplug / PCI: Don't scan bridges managed by native hotplug")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Valerio Passini <passini.valerio@xxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
> I was able to reproduce this without access to the affected system by
> forcing ACPI core to send Notify() to the TBT root port like this:
> 
> void acpi_notify_rp(void)
> {
> 	struct acpi_device *adev;
> 	acpi_handle handle;
> 
> 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB.PCI0.RP17", &handle)))
> 		return;
> 
> 	if (acpi_bus_get_device(handle, &adev))
> 		return;
> 
> 	dev_info(&adev->dev, "queueing hotplug\n");
> 	acpiphp_hotplug_notify(adev, ACPI_NOTIFY_BUS_CHECK);
> }
> 
> and calling it from acpi_init() directly after acpi_ec_init().
> 
>  drivers/pci/hotplug/acpiphp_glue.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index e4c46637f32f..b3869951c0eb 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -449,8 +449,15 @@ static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
>  
>  	/* Scan non-hotplug bridges that need to be reconfigured */
>  	for_each_pci_bridge(dev, bus) {
> -		if (!hotplug_is_native(dev))
> -			max = pci_scan_bridge(bus, dev, max, 1);
> +		if (hotplug_is_native(dev))
> +			continue;
> +
> +		max = pci_scan_bridge(bus, dev, max, 1);
> +		if (dev->subordinate) {
> +			pcibios_resource_survey_bus(dev->subordinate);
> +			pci_bus_size_bridges(dev->subordinate);
> +			pci_bus_assign_resources(dev->subordinate);
> +		}
>  	}
>  }
>  
> @@ -480,7 +487,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  			if (PCI_SLOT(dev->devfn) == slot->device)
>  				acpiphp_native_scan_bridge(dev);
>  		}
> -		pci_assign_unassigned_bridge_resources(bus->self);
>  	} else {
>  		LIST_HEAD(add_list);
>  		int max, pass;
> -- 
> 2.23.0
> 



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux