Re: [PATCH v5 3/4] PCI/ASPM: add sysfs attributes for controlling ASPM link states

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Oct 02, 2019 at 11:10:55PM +0200, Heiner Kallweit wrote:
> On 02.10.2019 21:55, Bjorn Helgaas wrote:
> > On Sun, Sep 29, 2019 at 07:15:05PM +0200, Heiner Kallweit wrote:
> >> On 07.09.2019 22:32, Bjorn Helgaas wrote:
> >>> On Sat, Aug 31, 2019 at 10:20:47PM +0200, Heiner Kallweit wrote:

> >>>> +static struct pcie_link_state *aspm_get_parent_link(struct pci_dev *pdev)
> >>>
> >>> I know the ASPM code is pretty confused, but I don't think "parent
> >>> link" really makes sense.  "Parent" implies a parent/child
> >>> relationship, but a link doesn't have a parent or a child; it only has
> >>> an upstream end and a downstream end.
> >>>
> >> I basically copied this "parent" stuff from __pci_disable_link_state.
> >> Fine with me to change the naming.
> >> What confuses me a little is that we have different versions of getting
> >> the pcie_link_state for a pci_dev in:
> >>
> >> - this new function of mine
> >> - __pci_disable_link_state
> >> - pcie_aspm_enabled
> >>
> >> The latter uses pci_upstream_bridge instead of accessing pdev->bus->self
> >> directly and doesn't include the call to pcie_downstream_port.
> >> I wonder whether the functionality could be factored out to a generic
> >> helper that works in all these places.
> > 
> > Definitely.  I think your pcie_aspm_get_link() (from the v6 patch)
> > could be used directly in those places.  You could add a new patch
> > that just adds pcie_aspm_get_link() and uses it.
> > 
> 
> OK
> 
> >>>> +{
> >>>> +	struct pci_dev *parent = pdev->bus->self;
> >>>> +
> >>>> +	if (pcie_downstream_port(pdev))
> >>>> +		parent = pdev;
> >>>> +
> >>>> +	return parent ? parent->link_state : NULL;
> >>>> +}
> >>>> +
> >>>> +static bool pcie_check_valid_aspm_endpoint(struct pci_dev *pdev)
> >>>> +{
> >>>> +	struct pcie_link_state *link;
> >>>> +
> >>>> +	if (!pci_is_pcie(pdev) || pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT)
> >>>
> >>> Do you intend to exclude other Upstream Ports like Legacy Endpoints,
> >>> Upstream Switch Ports, and PCIe-to-PCI/PCI-X Bridges?  They also have
> >>> a link leading to them, so we might want them to have knobs as well.
> >>> Or if we don't want the knobs, a comment about why not would be
> >>> useful.
> >>>
> >> My use case is about endpoints only and I'm not really a PCI expert.
> >> Based on your list in addition to PCI_EXP_TYPE_ENDPOINT we'd enable
> >> the ASPM sysfs fils for:
> >> - PCI_EXP_TYPE_LEG_END
> >> - PCI_EXP_TYPE_UPSTREAM
> >> - PCI_EXP_TYPE_PCI_BRIDGE
> >> - PCI_EXP_TYPE_PCIE_BRIDGE
> >> If you can confirm the list I'd extend my patch accordingly.
> > 
> > Yes, I think the list would be right, but looking at this again, I
> > don't think you need this function at all -- you can just use
> > pcie_aspm_get_link().  Then aspm_ctrl_attrs_are_visible() uses exactly
> > the same test as the show/store functions.  Actually, I think then you
> > could omit the "if (!link)" tests from the show/store functions
> > because those functions can never be called unless
> > aspm_ctrl_attrs_are_visible() found a link.
> > 
> Right, the !link checks can be removed from the show/store functions.
> In pcie_is_aspm_dev() I think we need to check at least whether
> device is PCIe and whether link is ASPM-capable. Making the sysfs
> attributes visible for a non-PCIe device doesn't make sense,
> the same applies to PCIe devices with a link that is not ASPM-capable.

I agree we don't want these attributes visible for non-PCIe or
non-ASPM-capable situations, but I think you can do this:

  static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
  {
    struct pci_dev *bridge = pci_upstream_bridge(pdev);

    if (bridge)
      return bridge->link_state;

    return NULL;
  }

  static umode_t aspm_ctrl_attrs_are_visible(...)
  {
    ...
    if (pcie_aspm_get_link(pdev))
      return a->mode;

    return 0;
  }

We can rely on pcie_aspm_init_link_state() to only set
bridge->link_state if the devices on both ends of the link are PCIe
and support ASPM.