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:
>>>> Background of this extension is a problem with the r8169 network driver.
>>>> Several combinations of board chipsets and network chip versions have
>>>> problems if ASPM is enabled, therefore we have to disable ASPM per default.
>>>> However especially on notebooks ASPM can provide significant power-saving,
>>>> therefore we want to give users the option to enable ASPM. With the new
>>>> sysfs attributes users can control which ASPM link-states are
>>>> enabled/disabled.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> v2:
>>>> - use a dedicated sysfs attribute per link state
>>>> - allow separate control of ASPM and PCI PM L1 sub-states
>>>> v3:
>>>> - statically allocate the attribute group
>>>> - replace snprintf with printf
>>>> - base on top of "PCI: Make pcie_downstream_port() available outside of access.c"
>>>> v4:
>>>> - add call to sysfs_update_group because is_visible callback returns false
>>>> always at file creation time
>>>> - simplify code a little
>>>> v5:
>>>> - rebased to latest pci/next
>>>> ---
>>>> Documentation/ABI/testing/sysfs-bus-pci | 13 ++
>>>> drivers/pci/pci-sysfs.c | 7 +
>>>> drivers/pci/pci.h | 4 +
>>>> drivers/pci/pcie/aspm.c | 184 ++++++++++++++++++++++++
>>>> 4 files changed, 208 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>>>> index 8bfee557e..49249a165 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>>>> @@ -347,3 +347,16 @@ Description:
>>>> If the device has any Peer-to-Peer memory registered, this
>>>> file contains a '1' if the memory has been published for
>>>> use outside the driver that owns the device.
>>>> +
>>>> +What /sys/bus/pci/devices/.../aspm/aspm_l0s
>>>> +What /sys/bus/pci/devices/.../aspm/aspm_l1
>>>> +What /sys/bus/pci/devices/.../aspm/aspm_l1_1
>>>> +What /sys/bus/pci/devices/.../aspm/aspm_l1_2
>>>> +What /sys/bus/pci/devices/.../aspm/aspm_l1_1_pcipm
>>>> +What /sys/bus/pci/devices/.../aspm/aspm_l1_2_pcipm
>>>> +What /sys/bus/pci/devices/.../aspm/aspm_clkpm
>>>> +date: August 2019
>
> I didn't notice this before, but I wonder if one "aspm" in these paths
> would be enough? E.g., /sys/bus/pci/devices/.../aspm/l0s?
>
Yes, that should be fine.
>>>> @@ -1315,6 +1315,10 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>>>>
>>>> pcie_vpd_create_sysfs_dev_files(dev);
>>>> pcie_aspm_create_sysfs_dev_files(dev);
>>>> +#ifdef CONFIG_PCIEASPM
>>>> + /* update visibility of attributes in this group */
>>>> + sysfs_update_group(&dev->dev.kobj, &aspm_ctrl_attr_group);
>>>> +#endif
>>>
>>> Isn't there a way to do this in drivers/pci/pcie/aspm.c somehow,
>>> without using sysfs_update_group()? There are only three callers of
>>> it in the tree, and I'd be surprised if ASPM is unique enough to have
>>> to be the fourth.
>>>
>> At least I didn't find any. Reason seems to be the following:
>> Static sysfs files are created in pci_scan_single_device ->
>> pci_device_add. And pci_scan_slot calls pci_scan_single_device
>> before calling pcie_aspm_init_link_state(bus->self).
>> Means the pcie_link_state doesn't exist yet and we have to update
>> visibility of the ASPM sysfs files later.
>
> Ah, I see. I think it's this call graph:
>
> pci_scan_slot
> pci_scan_single_device
> pci_scan_device
> pci_device_add
> pci_init_capabilities
> device_add
> device_add_attrs
> device_add_groups(dev->type->groups)
> sysfs_create_groups # <-- sysfs files created
> pcie_aspm_init_link_state(bridge) # <-- link_states allocated
>
> I think this part of the ASPM code is a little bit broken -- we wait
> to initialize ASPM until we've enumerated all the devices on the link.
> I think it would be better to initialize it somewhere in
> pci_device_add(), maybe pci_init_capabilities(), which would solve
> this ordering problem. That's a pretty big project that can be done
> later.
>
> But I *think* we should be able to at least move the
> sysfs_update_group() to the end of pcie_aspm_init_link_state(). We'd
> have to iterate over the subordinate->devices, but it would at least
> be in the ASPM code where we'll see it if/when we rework the
> initialization.
>
OK
>>>> +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.
> Bjorn
>
Heiner
