[PATCH 1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver

[PATCH 1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver

[Huacai Chen]

As Bjorn Helgaas said, portdrv can only be built statically (not as a
module), so the .remove() method in pcie_portdriver is useless. So just
remove it.

BTW, rename pcie_portdrv_remove() to pcie_portdrv_shutdown() since it
is only used by the .shutdown() method now.

Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
 drivers/pci/pcie/portdrv_pci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 3a3ce40..4e0af0f 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -134,7 +134,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	return 0;
 }
 
-static void pcie_portdrv_remove(struct pci_dev *dev)
+static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
 	if (pci_bridge_d3_possible(dev)) {
 		pm_runtime_forbid(&dev->dev);
@@ -210,8 +210,7 @@ static struct pci_driver pcie_portdriver = {
 	.id_table	= &port_pci_ids[0],
 
 	.probe		= pcie_portdrv_probe,
-	.remove		= pcie_portdrv_remove,
-	.shutdown	= pcie_portdrv_remove,
+	.shutdown	= pcie_portdrv_shutdown,
 
 	.err_handler	= &pcie_portdrv_err_handler,
 
-- 
2.7.0

[PATCH 2/2] PCI/portdrv: Don't disable pci device during shutdown

[Huacai Chen]

Don't call pci_disable_device() in pcie_port_device_remove() during the
portdrv's shutdown. This can avoid some poweroff/reboot failures.

The poweroff/reboot failures can easily reproduce on Loongson platforms.
I think this is not a Loongson-specific problem, instead, is a problem
related to some specific PCI hosts. On some x86 platforms, radeon/amdgpu
devices can cause the same problem, and commit faefba95c9e8ca3a523831c2e
("drm/amdgpu: just suspend the hw on pci shutdown") can resolve it.

Radeon driver is more difficult than amdgpu due to its confusing symbol
names, and I have maintained an out-of-tree patch for a long time [1].
Recently, we found more and more devices can cause the same problem, and
it is very difficult to modify all problematic drivers as radeon/amdgpu
does. So, I think modify the PCIe port driver is a simple and effective
way.

[1] https://github.com/chenhuacai/linux/commit/6612f9c1fc290d42a14618ce9a7d03014d8ebb1a

Signed-off-by: Huacai Chen <chenhc@lemote.com>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 drivers/pci/pcie/portdrv_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522..1991aca 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -491,7 +491,6 @@ void pcie_port_device_remove(struct pci_dev *dev)
 {
 	device_for_each_child(&dev->dev, NULL, remove_iter);
 	pci_free_irq_vectors(dev);
-	pci_disable_device(dev);
 }
 
 /**
-- 
2.7.0

Re: [PATCH 2/2] PCI/portdrv: Don't disable pci device during shutdown

[Bjorn Helgaas]

On Fri, Sep 11, 2020 at 06:09:37PM +0800, Huacai Chen wrote:
> Don't call pci_disable_device() in pcie_port_device_remove() during the
> portdrv's shutdown. This can avoid some poweroff/reboot failures.
> 
> The poweroff/reboot failures can easily reproduce on Loongson platforms.
> I think this is not a Loongson-specific problem, instead, is a problem
> related to some specific PCI hosts. On some x86 platforms, radeon/amdgpu
> devices can cause the same problem, and commit faefba95c9e8ca3a523831c2e
> ("drm/amdgpu: just suspend the hw on pci shutdown") can resolve it.
> 
> Radeon driver is more difficult than amdgpu due to its confusing symbol
> names, and I have maintained an out-of-tree patch for a long time [1].
> Recently, we found more and more devices can cause the same problem, and
> it is very difficult to modify all problematic drivers as radeon/amdgpu
> does. So, I think modify the PCIe port driver is a simple and effective
> way.

This needs to explain in more detail what the failure is and how this
patch fixes it.

The main thing pci_disable_device() does is turn off bus mastering, so
I assume this has to do with DMA during shutdown or reboot.  This
change is in portdrv, so it affects PCIe Root Ports and Switch Ports,
which of course are type 1 (bridge) devices.  Clearing
PCI_COMMAND_MASTER on bridges prevents them from forwarding memory or
I/O requests in the upstream direction, i.e., it prevents DMA from
devices below the bridge.

But if the problem is DMA, the same problem may occur with Root
Complex Integrated Endpoints or conventional PCI devices, since
portdrv may not be involved in those topologies.

> [1] https://github.com/chenhuacai/linux/commit/6612f9c1fc290d42a14618ce9a7d03014d8ebb1a
> 
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  drivers/pci/pcie/portdrv_core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 50a9522..1991aca 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -491,7 +491,6 @@ void pcie_port_device_remove(struct pci_dev *dev)
>  {
>  	device_for_each_child(&dev->dev, NULL, remove_iter);
>  	pci_free_irq_vectors(dev);
> -	pci_disable_device(dev);
>  }
>  
>  /**
> -- 
> 2.7.0
> 

RE: [PATCH 2/2] PCI/portdrv: Don't disable pci device during shutdown

[Deucher, Alexander]

[AMD Public Use]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Sunday, September 13, 2020 12:05 PM
> To: Huacai Chen <chenhc@lemote.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; linux-pci@vger.kernel.org; Huacai Chen
> <chenhuacai@gmail.com>; Jiaxun Yang <jiaxun.yang@flygoat.com>; Tiezhu
> Yang <yangtiezhu@loongson.cn>
> Subject: Re: [PATCH 2/2] PCI/portdrv: Don't disable pci device during
> shutdown
> 
> On Fri, Sep 11, 2020 at 06:09:37PM +0800, Huacai Chen wrote:
> > Don't call pci_disable_device() in pcie_port_device_remove() during
> > the portdrv's shutdown. This can avoid some poweroff/reboot failures.
> >
> > The poweroff/reboot failures can easily reproduce on Loongson platforms.
> > I think this is not a Loongson-specific problem, instead, is a problem
> > related to some specific PCI hosts. On some x86 platforms,
> > radeon/amdgpu devices can cause the same problem, and commit
> > faefba95c9e8ca3a523831c2e
> > ("drm/amdgpu: just suspend the hw on pci shutdown") can resolve it.
> >
> > Radeon driver is more difficult than amdgpu due to its confusing
> > symbol names, and I have maintained an out-of-tree patch for a long time
> [1].
> > Recently, we found more and more devices can cause the same problem,
> > and it is very difficult to modify all problematic drivers as
> > radeon/amdgpu does. So, I think modify the PCIe port driver is a
> > simple and effective way.
> 
> This needs to explain in more detail what the failure is and how this patch
> fixes it.
> 
> The main thing pci_disable_device() does is turn off bus mastering, so I
> assume this has to do with DMA during shutdown or reboot.  This change is in
> portdrv, so it affects PCIe Root Ports and Switch Ports, which of course are
> type 1 (bridge) devices.  Clearing PCI_COMMAND_MASTER on bridges
> prevents them from forwarding memory or I/O requests in the upstream
> direction, i.e., it prevents DMA from devices below the bridge.
> 
> But if the problem is DMA, the same problem may occur with Root Complex
> Integrated Endpoints or conventional PCI devices, since portdrv may not be
> involved in those topologies.

I'm not sure I understand what the point of this patch is.  Isn't the whole point of the shutdown callback to cleanly tear down whatever tasks are happening on the device?  It could be DMA or stuff happening on the device itself (e.g., microcontrollers on the devices, etc.).  Most of the complications in GPU drivers are due to the lifetime issues between drm and other subsystems.

Alex

> 
> > [1]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> >
> ub.com%2Fchenhuacai%2Flinux%2Fcommit%2F6612f9c1fc290d42a14618ce9a
> 7d030
> >
> 14d8ebb1a&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C1cc
> 5cca01b5
> >
> 74485a0aa08d857feb88e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C
> 0%7C637
> >
> 356098841869775&amp;sdata=HJmniTV2jJLEiOh3UCFpqPuGeq38y6crax2ahZa
> 5Eqc%
> > 3D&amp;reserved=0
> >
> > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> > ---
> >  drivers/pci/pcie/portdrv_core.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/pci/pcie/portdrv_core.c
> > b/drivers/pci/pcie/portdrv_core.c index 50a9522..1991aca 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -491,7 +491,6 @@ void pcie_port_device_remove(struct pci_dev
> *dev)
> > {
> >  	device_for_each_child(&dev->dev, NULL, remove_iter);
> >  	pci_free_irq_vectors(dev);
> > -	pci_disable_device(dev);
> >  }
> >
> >  /**
> > --
> > 2.7.0
> >

Re: [PATCH 1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver

[Lukas Wunner]

On Fri, Sep 11, 2020 at 06:09:36PM +0800, Huacai Chen wrote:
> As Bjorn Helgaas said, portdrv can only be built statically (not as a
> module), so the .remove() method in pcie_portdriver is useless. So just
> remove it.

No, PCIe switches (containing upstream and downstream PCIe ports)
can be hot-plugged and hot-removed at runtime.  Every Thunderbolt
device contains a PCIe switch and is hot-pluggable.  We do want to
clean up a hot-removed PCIe port properly.

Thanks,

Lukas

> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -134,7 +134,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	return 0;
>  }
>  
> -static void pcie_portdrv_remove(struct pci_dev *dev)
> +static void pcie_portdrv_shutdown(struct pci_dev *dev)
>  {
>  	if (pci_bridge_d3_possible(dev)) {
>  		pm_runtime_forbid(&dev->dev);
> @@ -210,8 +210,7 @@ static struct pci_driver pcie_portdriver = {
>  	.id_table	= &port_pci_ids[0],
>  
>  	.probe		= pcie_portdrv_probe,
> -	.remove		= pcie_portdrv_remove,
> -	.shutdown	= pcie_portdrv_remove,
> +	.shutdown	= pcie_portdrv_shutdown,
>  
>  	.err_handler	= &pcie_portdrv_err_handler,

Re: [PATCH 1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver

[Bjorn Helgaas]

On Sun, Sep 13, 2020 at 07:01:29AM +0200, Lukas Wunner wrote:
> On Fri, Sep 11, 2020 at 06:09:36PM +0800, Huacai Chen wrote:
> > As Bjorn Helgaas said, portdrv can only be built statically (not as a
> > module), so the .remove() method in pcie_portdriver is useless. So just
> > remove it.
> 
> No, PCIe switches (containing upstream and downstream PCIe ports)
> can be hot-plugged and hot-removed at runtime.  Every Thunderbolt
> device contains a PCIe switch and is hot-pluggable.  We do want to
> clean up a hot-removed PCIe port properly.

Right, sorry, I was thinking only of driver unbinding, not of device
removal.  Sorry to have wasted your time.

> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -134,7 +134,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >  	return 0;
> >  }
> >  
> > -static void pcie_portdrv_remove(struct pci_dev *dev)
> > +static void pcie_portdrv_shutdown(struct pci_dev *dev)
> >  {
> >  	if (pci_bridge_d3_possible(dev)) {
> >  		pm_runtime_forbid(&dev->dev);
> > @@ -210,8 +210,7 @@ static struct pci_driver pcie_portdriver = {
> >  	.id_table	= &port_pci_ids[0],
> >  
> >  	.probe		= pcie_portdrv_probe,
> > -	.remove		= pcie_portdrv_remove,
> > -	.shutdown	= pcie_portdrv_remove,
> > +	.shutdown	= pcie_portdrv_shutdown,
> >  
> >  	.err_handler	= &pcie_portdrv_err_handler,

RE: [PATCH 1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver

[Deucher, Alexander]

[AMD Public Use]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Sunday, September 13, 2020 11:43 AM
> To: Lukas Wunner <lukas@wunner.de>
> Cc: Huacai Chen <chenhc@lemote.com>; Bjorn Helgaas
> <bhelgaas@google.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; linux-pci@vger.kernel.org; Huacai Chen
> <chenhuacai@gmail.com>; Jiaxun Yang <jiaxun.yang@flygoat.com>
> Subject: Re: [PATCH 1/2] PCI/portdrv: Remove the .remove() method in
> pcie_portdriver
> 
> On Sun, Sep 13, 2020 at 07:01:29AM +0200, Lukas Wunner wrote:
> > On Fri, Sep 11, 2020 at 06:09:36PM +0800, Huacai Chen wrote:
> > > As Bjorn Helgaas said, portdrv can only be built statically (not as
> > > a module), so the .remove() method in pcie_portdriver is useless. So
> > > just remove it.
> >
> > No, PCIe switches (containing upstream and downstream PCIe ports) can
> > be hot-plugged and hot-removed at runtime.  Every Thunderbolt device
> > contains a PCIe switch and is hot-pluggable.  We do want to clean up a
> > hot-removed PCIe port properly.
> 
> Right, sorry, I was thinking only of driver unbinding, not of device removal.
> Sorry to have wasted your time.
> 

FWIW, our newer GPUs have both upstream and downstream ports that are part of the device.

Slightly off topic, but does the current pm code handle these cases correctly?  ACPI related power handling doesn't seem to work correctly for these devices in laptops where the GPU power control is handled by ACPI.

Alex

> > > --- a/drivers/pci/pcie/portdrv_pci.c
> > > +++ b/drivers/pci/pcie/portdrv_pci.c
> > > @@ -134,7 +134,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > >  	return 0;
> > >  }
> > >
> > > -static void pcie_portdrv_remove(struct pci_dev *dev)
> > > +static void pcie_portdrv_shutdown(struct pci_dev *dev)
> > >  {
> > >  	if (pci_bridge_d3_possible(dev)) {
> > >  		pm_runtime_forbid(&dev->dev);
> > > @@ -210,8 +210,7 @@ static struct pci_driver pcie_portdriver = {
> > >  	.id_table	= &port_pci_ids[0],
> > >
> > >  	.probe		= pcie_portdrv_probe,
> > > -	.remove		= pcie_portdrv_remove,
> > > -	.shutdown	= pcie_portdrv_remove,
> > > +	.shutdown	= pcie_portdrv_shutdown,
> > >
> > >  	.err_handler	= &pcie_portdrv_err_handler,

Re: [PATCH 1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver

[Bjorn Helgaas]

[+cc Rafael]

On Mon, Sep 14, 2020 at 08:34:03PM +0000, Deucher, Alexander wrote:

> FWIW, our newer GPUs have both upstream and downstream ports that
> are part of the device.
> 
> Slightly off topic, but does the current pm code handle these cases
> correctly?  ACPI related power handling doesn't seem to work
> correctly for these devices in laptops where the GPU power control
> is handled by ACPI.

I guess these newer GPUs basically have a PCIe switch embedded in
them?  The picture in my head is this:

	     +--------------------------------------+
  +----+     |+--------+   +----------+   +--------+|
  |Root+------+Switch  +---+Switch    +---+GPU     ||
  |Port|     ||Upstream|   |Downstream|   |Endpoint||
  +----+     ||Port    |   |Port      |   |        ||
	     |+--------+   +----------+   +--------+|
	     +--------------------------------------+

Is that accurate?  If not, can you share "lspci -vv" output so we can
figure it out?

The PCI PM code is *supposed* to work with arbitrary hierarchies, so
assuming your devices are PCIe spec-compliant, it should work.

It sounds like ACPI PM is involved as well, and I can't speak to that
side at all.  But I know Rafael can :)

Bjorn

Re: [PATCH 1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver

[Lukas Wunner]

On Mon, Sep 14, 2020 at 08:34:03PM +0000, Deucher, Alexander wrote:
> FWIW, our newer GPUs have both upstream and downstream ports that are
> part of the device.
> 
> Slightly off topic, but does the current pm code handle these cases
> correctly?  ACPI related power handling doesn't seem to work correctly
> for these devices in laptops where the GPU power control is handled by
> ACPI.

PCIe ports are only suspended to D3 if pci_bridge_d3_possible() in
drivers/pci/pci.c returns true.  In particular, if the downstream
ports are hotplug-capable, they will *not* be suspended to D3 unless
"pcie_port_pm=force" is specified on the command line.  There was a
report of MCEs on Xeon-SP servers if hotplug ports were runtime suspended,
hence this rule.  Also, non-hotplug ports are not suspended if the BIOS
is older than 2015.

If the downstream ports are not suspended, by consequence the upstream
port above them isn't either.  So if the GPU is powered down by an ACPI
_PR3 method for the upstream port, that method may not be executed.

I think the _PR3 method for GPUs was located in the Root Port's namespace
so far.  If it's moved to a port below that, then it may be necessary to
adjust GPU driver code, e.g. where pci_pr3_present() is called (but that's
only called by nouveau and hda_intel.c AFAICS).

Thanks,

Lukas