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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.10.2019 00:10, Bjorn Helgaas wrote:
> 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.
> 
With the first one I agree. However there may be links where e.g. the
bridge doesn't support ASPM. One example is my small Zotac test system:

Intel Corporation Celeron N3350/Pentium N4200/Atom E3900 Series PCI Express Port
 LnkCap: Port #3, Speed 5GT/s, Width x1, ASPM not supported