| Message ID | 20190827134756.10807-1-kai.heng.feng@canonical.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/2] PCI: Add a helper to check Power Resource Requirements _PR3 existence | expand |
On Tue, 27 Aug 2019 15:47:55 +0200, Kai-Heng Feng wrote: > > A driver may want to know the existence of _PR3, to choose different > runtime suspend behavior. A user will be add in next patch. > > This is mostly the same as nouveau_pr3_present(). Then it'd be nice to clean up the nouveau part, too? thanks, Takashi
at 23:25, Takashi Iwai <tiwai@suse.de> wrote: > On Tue, 27 Aug 2019 15:47:55 +0200, > Kai-Heng Feng wrote: >> A driver may want to know the existence of _PR3, to choose different >> runtime suspend behavior. A user will be add in next patch. >> >> This is mostly the same as nouveau_pr3_present(). > > Then it'd be nice to clean up the nouveau part, too? nouveau_pr3_present() may call pci_d3cold_disable(), and my intention is to only check the presence of _PR3 (i.e. a dGPU) without touching anything. Kai-Heng > > > thanks, > > Takashi
[+cc Peter, Mika, Dave] https://lore.kernel.org/r/20190827134756.10807-1-kai.heng.feng@canonical.com On Wed, Aug 28, 2019 at 12:58:28AM +0800, Kai-Heng Feng wrote: > at 23:25, Takashi Iwai <tiwai@suse.de> wrote: > > On Tue, 27 Aug 2019 15:47:55 +0200, > > Kai-Heng Feng wrote: > > > A driver may want to know the existence of _PR3, to choose different > > > runtime suspend behavior. A user will be add in next patch. > > > > > > This is mostly the same as nouveau_pr3_present(). > > > > Then it'd be nice to clean up the nouveau part, too? > > nouveau_pr3_present() may call pci_d3cold_disable(), and my intention is to > only check the presence of _PR3 (i.e. a dGPU) without touching anything. It looks like Peter added that code with 279cf3f23870 ("drm/nouveau/acpi: use DSM if bridge does not support D3cold"). I don't understand the larger picture, but it is somewhat surprising that nouveau_pr3_present() *looks* like a simple predicate with no side-effects, but in fact it disables the use of D3cold in some cases. If the disable were moved to the caller, Kai-Heng's new interface could be used both places.
On Tue, Aug 27, 2019 at 05:13:21PM -0500, Bjorn Helgaas wrote: > [+cc Peter, Mika, Dave] > > https://lore.kernel.org/r/20190827134756.10807-1-kai.heng.feng@canonical.com > > On Wed, Aug 28, 2019 at 12:58:28AM +0800, Kai-Heng Feng wrote: > > at 23:25, Takashi Iwai <tiwai@suse.de> wrote: > > > On Tue, 27 Aug 2019 15:47:55 +0200, > > > Kai-Heng Feng wrote: > > > > A driver may want to know the existence of _PR3, to choose different > > > > runtime suspend behavior. A user will be add in next patch. > > > > > > > > This is mostly the same as nouveau_pr3_present(). > > > > > > Then it'd be nice to clean up the nouveau part, too? > > > > nouveau_pr3_present() may call pci_d3cold_disable(), and my intention is to > > only check the presence of _PR3 (i.e. a dGPU) without touching anything. > > It looks like Peter added that code with 279cf3f23870 > ("drm/nouveau/acpi: use DSM if bridge does not support D3cold"). > > I don't understand the larger picture, but it is somewhat surprising > that nouveau_pr3_present() *looks* like a simple predicate with no > side-effects, but in fact it disables the use of D3cold in some cases. The reason for disabling _PR3 from that point on is because mixing the ACPI firmware code that uses power resources (_PR3) with the legacy _DSM/_PS0/_PS3 methods to manage power states could break as that combination is unlikely to be supported nor tested by firmware authors. If a user sets /sys/bus/pci/devices/.../d3cold_allowed to 0, then the pci_d3cold_disable call ensures that this action is remembered and prevents power resources from being used again. For example, compare this power resource _OFF code: https://github.com/Lekensteyn/acpi-stuff/blob/b55f6bdb/dsl/Clevo_P651RA/ssdt3.dsl#L454-L471 with this legacy _PS0/_PS3 code: https://github.com/Lekensteyn/acpi-stuff/blob/b55f6bdb/dsl/Clevo_P651RA/ssdt7.dsl#L113-L142 The power resource code checks the "MSD3" variable to check whether a transition to OFF is required while the legacy _PS3 checks "DGPS". The sequence PG00._OFF followed by _DSM (to to change "OPCE") and _PS3 might trigger some device-specific code twice and could lead to lockups (infinite loops polling for power state) or worse. I am not sure if I have ever tested this scenario however. > If the disable were moved to the caller, Kai-Heng's new interface > could be used both places. Moving the pci_d3cold_disable call to the caller looks reasonable to me. After the first patch gets merged, nouveau could use something like: *has_pr3 = pci_pr3_present(pdev); if (*has_pr3 && !pdev->bridge_d3) { /* * ... */ pci_d3cold_disable(pdev); *has_pr3 = false; } For the 1/2 patch, Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Maybe: PCI: Add pci_pr3_present() to check for Power Resources for D3hot On Tue, Aug 27, 2019 at 09:47:55PM +0800, Kai-Heng Feng wrote: > A driver may want to know the existence of _PR3, to choose different > runtime suspend behavior. A user will be add in next patch. Maybe include something like this in the commit lot? Add pci_pr3_present() to check whether the platform supplies _PR3 to tell us which power resources the device depends on when in D3hot. > This is mostly the same as nouveau_pr3_present(). > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/pci/pci.c | 20 ++++++++++++++++++++ > include/linux/pci.h | 2 ++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 1b27b5af3d55..776af15b92c2 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5856,6 +5856,26 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode, > return 0; > } > > +bool pci_pr3_present(struct pci_dev *pdev) > +{ > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > + struct acpi_device *parent_adev; > + > + if (acpi_disabled) > + return false; > + > + if (!parent_pdev) > + return false; > + > + parent_adev = ACPI_COMPANION(&parent_pdev->dev); > + if (!parent_adev) > + return false; > + > + return parent_adev->power.flags.power_resources && > + acpi_has_method(parent_adev->handle, "_PR3"); I think this is generally OK, but it doesn't actually check whether *pdev* has a _PR3; it checks whether pdev's *parent* does. So does that mean this is dependent on the GPU topology, i.e., does it assume that there is an upstream bridge and that power for everything under that bridge can be managed together? I'm wondering whether the "parent_pdev = pci_upstream_bridge()" part should be in the caller rather than in pci_pr3_present()? I can't connect any of the dots from _PR3 through to "need_eld_notify_link" (whatever "eld" is :)) and the uses of hda_intel.need_eld_notify_link (and needs_eld_notify_link()). But that's beyond the scope of *this* patch and it makes sense that you do want to discover the _PR3 existence, so I'm fine with this once we figure out the pdev vs parent question. > +} > +EXPORT_SYMBOL_GPL(pci_pr3_present); > + > /** > * pci_add_dma_alias - Add a DMA devfn alias for a device > * @dev: the PCI device for which alias is added > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 82e4cd1b7ac3..9b6f7b67fac9 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -2348,9 +2348,11 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus); > > void > pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *)); > +bool pci_pr3_present(struct pci_dev *pdev); > #else > static inline struct irq_domain * > pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; } > +static bool pci_pr3_present(struct pci_dev *pdev) { return false; } > #endif > > #ifdef CONFIG_EEH > -- > 2.17.1 >
Hi Bjorn, I didn't find your reply in my mailbox earlier. On Mon, Sep 9, 2019 at 1:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > Maybe: > > PCI: Add pci_pr3_present() to check for Power Resources for D3hot Ok, this is a good title. > > On Tue, Aug 27, 2019 at 09:47:55PM +0800, Kai-Heng Feng wrote: > > A driver may want to know the existence of _PR3, to choose different > > runtime suspend behavior. A user will be add in next patch. > > Maybe include something like this in the commit lot? > > Add pci_pr3_present() to check whether the platform supplies _PR3 to > tell us which power resources the device depends on when in D3hot. Ok. > > > This is mostly the same as nouveau_pr3_present(). > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/pci/pci.c | 20 ++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 1b27b5af3d55..776af15b92c2 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -5856,6 +5856,26 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode, > > return 0; > > } > > > > +bool pci_pr3_present(struct pci_dev *pdev) > > +{ > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > > + struct acpi_device *parent_adev; > > + > > + if (acpi_disabled) > > + return false; > > + > > + if (!parent_pdev) > > + return false; > > + > > + parent_adev = ACPI_COMPANION(&parent_pdev->dev); > > + if (!parent_adev) > > + return false; > > + > > + return parent_adev->power.flags.power_resources && > > + acpi_has_method(parent_adev->handle, "_PR3"); > > I think this is generally OK, but it doesn't actually check whether > *pdev* has a _PR3; it checks whether pdev's *parent* does. So does > that mean this is dependent on the GPU topology, i.e., does it assume > that there is an upstream bridge and that power for everything under > that bridge can be managed together? Yes, the power resource is managed by its upstream port. > > I'm wondering whether the "parent_pdev = pci_upstream_bridge()" part > should be in the caller rather than in pci_pr3_present()? This will make the function more align to its name, but needs more work from caller side. How about rename the function to pci_upstream_pr3_present()? > > I can't connect any of the dots from _PR3 through to > "need_eld_notify_link" (whatever "eld" is :)) and the uses of > hda_intel.need_eld_notify_link (and needs_eld_notify_link()). > > But that's beyond the scope of *this* patch and it makes sense that > you do want to discover the _PR3 existence, so I'm fine with this once > we figure out the pdev vs parent question. Thanks for your review. Kai-Heng > > > +} > > +EXPORT_SYMBOL_GPL(pci_pr3_present); > > + > > /** > > * pci_add_dma_alias - Add a DMA devfn alias for a device > > * @dev: the PCI device for which alias is added > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 82e4cd1b7ac3..9b6f7b67fac9 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -2348,9 +2348,11 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus); > > > > void > > pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *)); > > +bool pci_pr3_present(struct pci_dev *pdev); > > #else > > static inline struct irq_domain * > > pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; } > > +static bool pci_pr3_present(struct pci_dev *pdev) { return false; } > > #endif > > > > #ifdef CONFIG_EEH > > -- > > 2.17.1 > >
[+cc Rafael] On Fri, Sep 20, 2019 at 01:23:20PM +0200, Kai-Heng Feng wrote: > On Mon, Sep 9, 2019 at 1:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > +bool pci_pr3_present(struct pci_dev *pdev) > > > +{ > > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > > > + struct acpi_device *parent_adev; > > > + > > > + if (acpi_disabled) > > > + return false; > > > + > > > + if (!parent_pdev) > > > + return false; > > > + > > > + parent_adev = ACPI_COMPANION(&parent_pdev->dev); > > > + if (!parent_adev) > > > + return false; > > > + > > > + return parent_adev->power.flags.power_resources && > > > + acpi_has_method(parent_adev->handle, "_PR3"); > > > > I think this is generally OK, but it doesn't actually check whether > > *pdev* has a _PR3; it checks whether pdev's *parent* does. So does > > that mean this is dependent on the GPU topology, i.e., does it assume > > that there is an upstream bridge and that power for everything under > > that bridge can be managed together? > > Yes, the power resource is managed by its upstream port. > > > I'm wondering whether the "parent_pdev = pci_upstream_bridge()" part > > should be in the caller rather than in pci_pr3_present()? > > This will make the function more align to its name, but needs more > work from caller side. > How about rename the function to pci_upstream_pr3_present()? I cc'd Rafael because he knows all about how this stuff works, and I don't. _PR3 is defined in terms of the device itself and the doc (ACPI v6.3, sec 7.3.11) doesn't mention any hierarchy. I assume it would be legal for firmware to supply a _PR3 for "pdev" as well as for "parent_pdev"? If that is legal, I think it would be appropriate for the caller to look up the upstream bridge. That way this interface could be used for both "pdev" and an upstream bridge. If we look up the bridge internally, we would have to add a second interface if we actually want to know about _PR3 for the device itself. > > I can't connect any of the dots from _PR3 through to > > "need_eld_notify_link" (whatever "eld" is :)) and the uses of > > hda_intel.need_eld_notify_link (and needs_eld_notify_link()). > > > > But that's beyond the scope of *this* patch and it makes sense that > > you do want to discover the _PR3 existence, so I'm fine with this once > > we figure out the pdev vs parent question. > > Thanks for your review. > > Kai-Heng > > > > > > +} > > > +EXPORT_SYMBOL_GPL(pci_pr3_present); > > > + > > > /** > > > * pci_add_dma_alias - Add a DMA devfn alias for a device > > > * @dev: the PCI device for which alias is added > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > > index 82e4cd1b7ac3..9b6f7b67fac9 100644 > > > --- a/include/linux/pci.h > > > +++ b/include/linux/pci.h > > > @@ -2348,9 +2348,11 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus); > > > > > > void > > > pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *)); > > > +bool pci_pr3_present(struct pci_dev *pdev); > > > #else > > > static inline struct irq_domain * > > > pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; } > > > +static bool pci_pr3_present(struct pci_dev *pdev) { return false; } > > > #endif > > > > > > #ifdef CONFIG_EEH > > > -- > > > 2.17.1 > > >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 1b27b5af3d55..776af15b92c2 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5856,6 +5856,26 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode, return 0; } +bool pci_pr3_present(struct pci_dev *pdev) +{ + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); + struct acpi_device *parent_adev; + + if (acpi_disabled) + return false; + + if (!parent_pdev) + return false; + + parent_adev = ACPI_COMPANION(&parent_pdev->dev); + if (!parent_adev) + return false; + + return parent_adev->power.flags.power_resources && + acpi_has_method(parent_adev->handle, "_PR3"); +} +EXPORT_SYMBOL_GPL(pci_pr3_present); + /** * pci_add_dma_alias - Add a DMA devfn alias for a device * @dev: the PCI device for which alias is added diff --git a/include/linux/pci.h b/include/linux/pci.h index 82e4cd1b7ac3..9b6f7b67fac9 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2348,9 +2348,11 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus); void pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *)); +bool pci_pr3_present(struct pci_dev *pdev); #else static inline struct irq_domain * pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; } +static bool pci_pr3_present(struct pci_dev *pdev) { return false; } #endif #ifdef CONFIG_EEH
A driver may want to know the existence of _PR3, to choose different runtime suspend behavior. A user will be add in next patch. This is mostly the same as nouveau_pr3_present(). Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/pci/pci.c | 20 ++++++++++++++++++++ include/linux/pci.h | 2 ++ 2 files changed, 22 insertions(+)