Re: [PATCH 1/2] PCI: Add a helper to check Power Resource Requirements _PR3 existence

[Kai-Heng Feng]

Hi Bjorn,

I didn't find your reply in my mailbox earlier.

On Mon, Sep 9, 2019 at 1:41 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >  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
> >