Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()

From: Bjorn Helgaas <helgaas@kernel.org>
To: "Limonciello, Mario" <Mario.Limonciello@amd.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	"Mehta, Sanju" <Sanju.Mehta@amd.com>,
	Lukas Wunner <lukas@wunner.de>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()
Date: Fri, 13 Jan 2023 11:51:36 -0600	[thread overview]
Message-ID: <20230113175136.GA1848594@bhelgaas> (raw)
In-Reply-To: <MN0PR12MB61016E8166D73C4BD7C720D4E2FD9@MN0PR12MB6101.namprd12.prod.outlook.com>

On Thu, Jan 12, 2023 at 10:45:03PM +0000, Limonciello, Mario wrote:
> [AMD Official Use Only - General]
> 
> 
> 
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Thursday, January 12, 2023 16:41
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; linux-pci@vger.kernel.org; Rafael J.
> > Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; Bjorn Helgaas
> > <bhelgaas@google.com>; Mika Westerberg
> > <mika.westerberg@linux.intel.com>; Mehta, Sanju <Sanju.Mehta@amd.com>;
> > Lukas Wunner <lukas@wunner.de>; Rafael J . Wysocki
> > <rafael.j.wysocki@intel.com>; linux-acpi@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Linux PM <linux-pm@vger.kernel.org>
> > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> > account in acpi_pci_bridge_d3(()
> > 
> > On Thu, Jan 12, 2023 at 10:09:21PM +0000, Limonciello, Mario wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > Sent: Thursday, January 12, 2023 16:02
> > > > To: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > > Cc: linux-pci@vger.kernel.org; Limonciello, Mario
> > > > <Mario.Limonciello@amd.com>; Rafael J. Wysocki <rafael@kernel.org>;
> > Len
> > > > Brown <lenb@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; Mika
> > > > Westerberg <mika.westerberg@linux.intel.com>; Mehta, Sanju
> > > > <Sanju.Mehta@amd.com>; Lukas Wunner <lukas@wunner.de>; Rafael J .
> > > > Wysocki <rafael.j.wysocki@intel.com>; linux-acpi@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; Linux PM <linux-pm@vger.kernel.org>
> > > > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> > > > account in acpi_pci_bridge_d3(()
> > > >
> > > > On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > It is generally questionable to allow a PCI bridge to go into D3 if
> > > > > it has _S0W returning D2 or a shallower power state, so modify
> > > > > acpi_pci_bridge_d3(() to always take the return value of _S0W for the
> > > > > target bridge into accout.  That is, make it return 'false' if _S0W
> > > > > returns D2 or a shallower power state for the target bridge regardless
> > > > > of its ancestor PCIe Root Port properties.  Of course, this also causes
> > > > > 'false' to be returned if the PCIe Root Port itself is the target and
> > > > > its _S0W returns D2 or a shallower power state.
> > > > >
> > > > > However, still allow bridges without _S0W that are power-manageable via
> > > > > ACPI to enter D3 to retain the current code behavior in that case.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-pci/20221031223356.32570-1-
> > > > mario.limonciello@amd.com/
> > > > > Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Applied to pci/pm for v6.3, thanks!
> > > >
> > > > It'd be great if we could include a short description of the problems
> > > > users might see.  I think the original problem was that on some AMD
> > > > systems we put a USB4 router in D3 when it should remain in D0.  And I
> > > > assume this means something doesn't wake up when it should?  Or maybe
> > > > we miss a hotplug event?
> > > >
> > > > If somebody has an example or some text, I'll add it to the commit
> > > > log.
> > >
> > > Here's a blurb for what happens on AMD side:
> > >
> > > When the platform is configured to not allow the PCIe port used for
> > > tunneling to wakeup from D3 it will runtime suspend into D0 and the
> > > USB4 controller which is a consumer will runtime suspend into D3.
> > >
> > > This inconsistency leads to failures to initialize PCIe tunnels for
> > > USB4 devices.
> > 
> > And what is J. Random User going to see?  DisplayPort not working
> > ever?  It works to begin with, but not after a suspend?  Devices in a
> > dock not being able to wake the system?
> > 
> > I don't really know what "PCIe tunnels for USB4 devices not being
> > initialized" means for me.  I want to know what a problem report from
> > a non-expert user might look like.
> 
> DP tunnels aren't affected, so monitors should still work.
> 
> In terms of what doesn't work it depends on the architecture of the
> the connected device.  Here's some concrete up-leveled examples:
> 
> USB4 docks contain that a PCIe network adapter and that adapter won't
> work when the dock is plugged in after the system boot.
> 
> USB4 docks that contain a USB network adapter should work properly,
> but downstream USB4 or TBT3 devices connected to that USB4 dock will
> not work when the device or dock is plugged in after the system boots.
> 
> TBT3 storage devices connected after the system has booted will not work.

Thanks, this is exactly the sort of thing I was looking for.  Since
they all mention "connected after boot," I assume the issue with the
current code is that a hotplug notification is being missed because a
bridge is in D3.

If the devices are present and enumerated at boot, there's no issue
with suspend/resume, right?

What do you think of the following possible text?  I don't want to be
overly specific because I don't think it's practical to list every
scenario.  We just need a hook to make people think "Hmm, I'm seeing a
dock issue; maybe this is the fix."

  This fixes problems where a hotplug notification is missed because a
  bridge is in D3.  That means hot-added devices such as USB4 docks
  (and the devices they contain) and Thunderbolt 3 devices may not
  work.