From: Heiner Kallweit <hkallweit1@gmail.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Frederick Lawler <fred@fredlawl.com>,
Bjorn Helgaas <bhelgaas@google.com>,
Rajat Jain <rajatja@google.com>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] PCI/ASPM: add sysfs attributes for controlling ASPM
Date: Thu, 22 Aug 2019 21:46:02 +0200 [thread overview]
Message-ID: <ff819b41-aab4-1bfd-e9f4-a7065ab789e4@gmail.com> (raw)
In-Reply-To: <e4a278f1-ac78-a2fd-ddfe-a4dd0d1685e3@gmail.com>
On 22.08.2019 21:15, Heiner Kallweit wrote:
> On 22.08.2019 15:05, Greg KH wrote:
>> On Wed, Aug 21, 2019 at 08:18:41PM +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@gmail.com>
>>> ---
>>> v2:
>>> - use a dedicated sysfs attribute per link state
>>> - allow separate control of ASPM and PCI PM L1 sub-states
>>> ---
>>> Documentation/ABI/testing/sysfs-bus-pci | 13 ++
>>> drivers/pci/pci.h | 8 +-
>>> drivers/pci/pcie/aspm.c | 263 +++++++++++++++++++++++-
>>> 3 files changed, 276 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>>> index 8bfee557e..38b565c30 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/.../power/aspm_l0s
>>> +What /sys/bus/pci/devices/.../power/aspm_l1
>>> +What /sys/bus/pci/devices/.../power/aspm_l1_1
>>> +What /sys/bus/pci/devices/.../power/aspm_l1_2
>>> +What /sys/bus/pci/devices/.../power/aspm_l1_1_pcipm
>>> +What /sys/bus/pci/devices/.../power/aspm_l1_2_pcipm
>>> +What /sys/bus/pci/devices/.../power/aspm_clkpm
>>
>> Wait, there already is a "power" subdirectory for all devices in the
>> system, are you adding another one, or files to that already-created
>> directory?
>>
> This is the standard "power" directory dynamically created by
> dpm_sysfs_add().
>
>>> +date: August 2019
>>> +Contact: Heiner Kallweit <hkallweit1@gmail.com>
>>> +Description: If ASPM is supported for an endpoint, then these files
>>> + can be used to disable or enable the individual
>>> + power management states.
>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>> index 3f126f808..e51c91f38 100644
>>> --- a/drivers/pci/pci.h
>>> +++ b/drivers/pci/pci.h
>>> @@ -525,17 +525,13 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
>>> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
>>> void pcie_aspm_pm_state_change(struct pci_dev *pdev);
>>> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>>> +void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
>>> +void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
>>> #else
>>> static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
>>> static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
>>> static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
>>> static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
>>> -#endif
>>> -
>>> -#ifdef CONFIG_PCIEASPM_DEBUG
>>> -void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
>>> -void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
>>> -#else
>>> static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev) { }
>>> static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev) { }
>>> #endif
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index 149b876c9..e7dfcd1bd 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -1275,38 +1275,297 @@ static ssize_t clk_ctl_store(struct device *dev,
>>>
>>> static DEVICE_ATTR_RW(link_state);
>>> static DEVICE_ATTR_RW(clk_ctl);
>>> +#endif
>>> +
>>> +static const char power_group[] = "power";
>>> +
>>> +static struct pcie_link_state *aspm_get_parent_link(struct pci_dev *pdev)
>>> +{
>>> + struct pci_dev *parent = pdev->bus->self;
>>> +
>>> + if (pdev->has_secondary_link)
>>> + 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_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT)
>>> + return false;
>>> +
>>> + link = aspm_get_parent_link(pdev);
>>> +
>>> + return link && link->aspm_capable;
>>> +}
>>> +
>>> +static ssize_t aspm_attr_show_common(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf, int state)
>>> +{
>>> + struct pci_dev *pdev = to_pci_dev(dev);
>>> + struct pcie_link_state *link;
>>> + int val;
>>> +
>>> + link = aspm_get_parent_link(pdev);
>>> + if (!link)
>>> + return -EOPNOTSUPP;
>>> +
>>> + mutex_lock(&aspm_lock);
>>> + val = !!(link->aspm_enabled & state);
>>> + mutex_unlock(&aspm_lock);
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>>
>> For sysfs files, you never need to use snprintf() as you "know" the size
>> of the buffer is big, and you are just printing out a single number. So
>> that can be cleaned up in all of these attribute files.
>>
>>> +}
>>> +
>>> +static ssize_t aspm_l0s_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + return aspm_attr_show_common(dev, attr, buf, ASPM_STATE_L0S);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + return aspm_attr_show_common(dev, attr, buf, ASPM_STATE_L1);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_1_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + return aspm_attr_show_common(dev, attr, buf, ASPM_STATE_L1_1);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_2_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + return aspm_attr_show_common(dev, attr, buf, ASPM_STATE_L1_2);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_1_pcipm_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + return aspm_attr_show_common(dev, attr, buf, ASPM_STATE_L1_1_PCIPM);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_2_pcipm_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + return aspm_attr_show_common(dev, attr, buf, ASPM_STATE_L1_2_PCIPM);
>>> +}
>>> +
>>> +static ssize_t aspm_attr_store_common(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len, int state)
>>> +{
>>> + struct pci_dev *pdev = to_pci_dev(dev);
>>> + struct pcie_link_state *link;
>>> + bool state_enable;
>>> +
>>> + if (aspm_disabled)
>>> + return -EPERM;
>>> +
>>> + link = aspm_get_parent_link(pdev);
>>> + if (!link)
>>> + return -EOPNOTSUPP;
>>> +
>>> + if (!(link->aspm_capable & state))
>>> + return -EOPNOTSUPP;
>>> +
>>> + if (strtobool(buf, &state_enable) < 0)
>>> + return -EINVAL;
>>> +
>>> + down_read(&pci_bus_sem);
>>> + mutex_lock(&aspm_lock);
>>> +
>>> + if (state_enable)
>>> + link->aspm_disable &= ~state;
>>> + else
>>> + link->aspm_disable |= state;
>>> +
>>> + if (link->aspm_disable & ASPM_STATE_L1)
>>> + link->aspm_disable |= ASPM_STATE_L1SS;
>>> +
>>> + pcie_config_aspm_link(link, policy_to_aspm_state(link));
>>
>> This function can't fail?
>>
> It has no return value.
>
>>> +
>>> + mutex_unlock(&aspm_lock);
>>> + up_read(&pci_bus_sem);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static ssize_t aspm_l0s_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + return aspm_attr_store_common(dev, attr, buf, len, ASPM_STATE_L0S);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + return aspm_attr_store_common(dev, attr, buf, len, ASPM_STATE_L1);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_1_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + return aspm_attr_store_common(dev, attr, buf, len, ASPM_STATE_L1_1);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_2_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + return aspm_attr_store_common(dev, attr, buf, len, ASPM_STATE_L1_2);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_1_pcipm_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + return aspm_attr_store_common(dev, attr, buf, len,
>>> + ASPM_STATE_L1_1_PCIPM);
>>> +}
>>> +
>>> +static ssize_t aspm_l1_2_pcipm_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + return aspm_attr_store_common(dev, attr, buf, len,
>>> + ASPM_STATE_L1_2_PCIPM);
>>> +}
>>> +
>>> +static ssize_t aspm_clkpm_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct pci_dev *pdev = to_pci_dev(dev);
>>> + struct pcie_link_state *link;
>>> + int val;
>>> +
>>> + link = aspm_get_parent_link(pdev);
>>> + if (!link)
>>> + return -EOPNOTSUPP;
>>> +
>>> + mutex_lock(&aspm_lock);
>>> + val = link->clkpm_enabled;
>>> + mutex_unlock(&aspm_lock);
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>>> +}
>>> +
>>> +static ssize_t aspm_clkpm_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct pci_dev *pdev = to_pci_dev(dev);
>>> + struct pcie_link_state *link;
>>> + bool state_enable;
>>> +
>>> + if (aspm_disabled)
>>> + return -EPERM;
>>> +
>>> + link = aspm_get_parent_link(pdev);
>>> + if (!link)
>>> + return -EOPNOTSUPP;
>>> +
>>> + if (!link->clkpm_capable)
>>> + return -EOPNOTSUPP;
>>> +
>>> + if (strtobool(buf, &state_enable) < 0)
>>> + return -EINVAL;
>>> +
>>> + down_read(&pci_bus_sem);
>>> + mutex_lock(&aspm_lock);
>>> +
>>> + link->clkpm_disable = !state_enable;
>>> + pcie_set_clkpm(link, policy_to_clkpm_state(link));
>>> +
>>> + mutex_unlock(&aspm_lock);
>>> + up_read(&pci_bus_sem);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RW(aspm_l0s);
>>> +static DEVICE_ATTR_RW(aspm_l1);
>>> +static DEVICE_ATTR_RW(aspm_l1_1);
>>> +static DEVICE_ATTR_RW(aspm_l1_2);
>>> +static DEVICE_ATTR_RW(aspm_l1_1_pcipm);
>>> +static DEVICE_ATTR_RW(aspm_l1_2_pcipm);
>>> +static DEVICE_ATTR_RW(aspm_clkpm);
>>>
>>> -static char power_group[] = "power";
>>> void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev)
>>> {
>>> struct pcie_link_state *link_state = pdev->link_state;
>>>
>>> + if (pcie_check_valid_aspm_endpoint(pdev)) {
>>> + sysfs_add_file_to_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l0s.attr, power_group);
>>> + sysfs_add_file_to_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1.attr, power_group);
>>> + sysfs_add_file_to_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_1.attr, power_group);
>>> + sysfs_add_file_to_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_2.attr, power_group);
>>> + sysfs_add_file_to_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_1_pcipm.attr, power_group);
>>> + sysfs_add_file_to_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_2_pcipm.attr, power_group);
>>> + sysfs_add_file_to_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_clkpm.attr, power_group);
>>
>> Huh? First of, if you are ever in a driver, and you have to call a
>> sysfs_* function, you know something is wrong.
>>
>> Why are you dynamically adding files to a group? These should all be
>> statically allocated and then at creation time you can dynamically
>> determine if you need to show them or not. Use the is_visable callback
>> for that.
>>
>
> The "power" group is dynamically created by the base driver core
> (dpm_sysfs_add). Not sure how the ASPM sysfs files could be statically
> allocated. We could add an attribute group to the "pci" bus_type,
> but this doesn't feel right.
> What we could do to make it simpler:
> Create an attribute group and then use sysfs_merge_group().
> But I see no way not to use sysfs_xxx functions. Also functions
> like device_add_groups() are just very simple wrappers around sysfs_
> functions.
>
> Alternatively we could not use the existing "power" group but create
> our own group with device_add_group(). But I don't see that we gain much.
>
After a little bit more checking:
We could statically add an attribute group to pci_dev_attr_groups,
similar to pcie_dev_attr_group.
>>> + }
>>> +
>>> if (!link_state)
>>> return;
>>>
>>> +#ifdef CONFIG_PCIEASPM_DEBUG
>>> if (link_state->aspm_support)
>>> sysfs_add_file_to_group(&pdev->dev.kobj,
>>> &dev_attr_link_state.attr, power_group);
>>> if (link_state->clkpm_capable)
>>> sysfs_add_file_to_group(&pdev->dev.kobj,
>>> &dev_attr_clk_ctl.attr, power_group);
>>
>> Same here.
>>
>>> +#endif
>>> }
>>>
>>> void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
>>> {
>>> struct pcie_link_state *link_state = pdev->link_state;
>>>
>>> + if (pcie_check_valid_aspm_endpoint(pdev)) {
>>> + sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l0s.attr, power_group);
>>> + sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1.attr, power_group);
>>> + sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_1.attr, power_group);
>>> + sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_2.attr, power_group);
>>> + sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_1_pcipm.attr, power_group);
>>> + sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_l1_2_pcipm.attr, power_group);
>>> + sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> + &dev_attr_aspm_clkpm.attr, power_group);
>>
>> And then you never have to do this type of messy logic at all.
>>
>> Ick.
>>
>>> + }
>>> +
>>> if (!link_state)
>>> return;
>>>
>>> +#ifdef CONFIG_PCIEASPM_DEBUG
>>> if (link_state->aspm_support)
>>> sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> &dev_attr_link_state.attr, power_group);
>>> if (link_state->clkpm_capable)
>>> sysfs_remove_file_from_group(&pdev->dev.kobj,
>>> &dev_attr_clk_ctl.attr, power_group);
>>
>> And you get to drop this nonsense as well.
>>
> Right, with the new attributes CONFIG_PCIEASPM_DEBUG and all
> related stuff could be removed most likely.
>
>> thanks,
>>
>> greg k-h
>>
>
> Heiner
>
prev parent reply other threads:[~2019-08-22 19:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-21 18:14 [PATCH v2 0/3] PCI/ASPM: add sysfs attributes for controlling ASPM Heiner Kallweit
2019-08-21 18:17 ` [PATCH v2 1/3] PCI/ASPM: add L1 sub-state support to pci_disable_link_state Heiner Kallweit
2019-08-21 18:17 ` [PATCH v2 2/3] PCI/ASPM: allow to re-enable Clock PM Heiner Kallweit
2019-08-21 18:18 ` [PATCH v2 3/3] PCI/ASPM: add sysfs attributes for controlling ASPM Heiner Kallweit
2019-08-22 13:05 ` Greg KH
2019-08-22 19:15 ` Heiner Kallweit
2019-08-22 19:46 ` Heiner Kallweit [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ff819b41-aab4-1bfd-e9f4-a7065ab789e4@gmail.com \
--to=hkallweit1@gmail.com \
--cc=bhelgaas@google.com \
--cc=fred@fredlawl.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-pci@vger.kernel.org \
--cc=rajatja@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).