| Message ID | 1599818977-25425-1-git-send-email-chenhc@lemote.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/2] PCI/portdrv: Remove the .remove() method in pcie_portdriver | expand |
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,
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,
[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,
[+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
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
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,
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(-)