On 02.04.2019 23:08, Rajat Jain wrote:
> On Tue, Apr 2, 2019 at 1:41 PM Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>>
>> On 02.04.2019 22:16, Florian Fainelli wrote:
>>> On 4/2/19 12:55 PM, Heiner Kallweit wrote:
>>>> There are numerous reports about different problems caused by ASPM
>>>> incompatibilities between certain network chip versions and board
>>>> chipsets. On the other hand on (especially mobile) systems where ASPM
>>>> works properly it can significantly contribute to power-saving and
>>>> increased battery runtime.
>>>> One problem so far to make ASPM configurable was to find an acceptable
>>>> way of configuration (e.g. module parameters are discouraged for that
>>>> purpose).
>>>>
>>>> As a new attempt let's switch off ASPM per default and make it
>>>> configurable by a sysfs attribute. The attribute is documented in
>>>> new file Documentation/networking/device_drivers/realtek/r8169.txt.
>>>
>>> I am not sure this is where it should be solved, there is definitively a
>>> device specific aspect to properly supporting the enabling of ASPM L0s,
>>> L1s etc, but the actual sysfs knobs should belong to the pci_device
>>> itself, since this is something that likely other drivers would want to
>>> be able to expose. You would probably want to work with the PCI
>>> maintainers to come up with a standard solution that applies beyond just
>>> r8169 since presumably there must be a gazillion of devices with the
>>> same issues.
>>>
>> Interesting point, so let me add Bjorn and linux-pci.
>>
>> Here this attribute only controls whether the device actively enters
>> ASPM states. I doesn't change anything on the other side of the link.
>>
>> Maybe it would be an option to add an attribute on device level in the
>> PCI core that reflects "ASPM allowed", or even in more detail which
>> ASPM states / sub-states are acceptable.
>
> CONFIG_PCIEASPM_DEBUG creates a link_state attribute on a per device
> basis, that could help achieve this to some extent. It does not
> indicate "what is allowed" but lets userspace control the ASPM to
> switch to certain states.
>
Indeed, this could be a good starting point. This functionality doesn't
seem to be too frequently used, else I think somebody had complained
already that link_state_show() displays a bitmap as decimal value.
Not sure whether link_state_store() is robust enough to deal with the
attempt to set unsupported modes. Maybe there's a reason why it's
explicitly a debug option. But if made a little bit more user-friendly
it could be what we need.
> Thanks,
>
> Rajat
>
>> Certain support for disabling
>> ASPM states is provided by pci_disable_link_state().
>> Not sure whether the device would have to be notified by the core that
>> certain states have been disabled. Maybe not because this is auto-
>> negotiated between the PCIe link partners.
>>
>> Based on the usage of pci_disable_link_state() not that many devices
>> seem to be effected. One good example may be Intel e1000e
>> (__e1000e_disable_aspm()).
>>
>>>>
>>>> Fixes: a99790bf5c7f ("r8169: Reinstate ASPM Support")
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>> ---
>>>> .../device_drivers/realtek/r8169.txt | 19 +++++
>>>> drivers/net/ethernet/realtek/r8169.c | 75 +++++++++++++++++--
>>>> 2 files changed, 86 insertions(+), 8 deletions(-)
>>>> create mode 100644 Documentation/networking/device_drivers/realtek/r8169.txt
>>>>
>>>> diff --git a/Documentation/networking/device_drivers/realtek/r8169.txt b/Documentation/networking/device_drivers/realtek/r8169.txt
>>>> new file mode 100644
>>>> index 000000000..669995d0c
>>>> --- /dev/null
>>>> +++ b/Documentation/networking/device_drivers/realtek/r8169.txt
>>>> @@ -0,0 +1,19 @@
>>>> +Written by Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>> +
>>>> +Version 04/02/2019
>>>> +
>>>> +Driver-specific sysfs attributes
>>>> +================================
>>>> +
>>>> +rtl_aspm (bool)
>>>> +---------------
>>>> +
>>>> +Certain combinations of network chip versions and board chipsets result in
>>>> +increased packet latency, PCIe errors, or significantly reduced network
>>>> +performance. Therefore ASPM is off by default. On the other hand ASPM can
>>>> +significantly contribute to power-saving and thus increased battery
>>>> +runtime on notebooks. Therefore this sysfs attribute allows to switch on
>>>> +ASPM on systems where ASPM works properly. The attribute accepts any form
>>>> +of bool value, e.g. 1/y/on. See also kerneldoc of kstrtobool().
>>>> +Note that the attribute is accessible only if interface is up.
>>>> +Else network chip and PCIe link may be powered-down and not reachable.
>>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>>> index 19efa88f3..e2de94dc3 100644
>>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>>> @@ -678,6 +678,8 @@ struct rtl8169_private {
>>>> struct work_struct work;
>>>> } wk;
>>>>
>>>> + unsigned aspm_supported:1;
>>>> + unsigned aspm_enabled:1;
>>>> unsigned irq_enabled:1;
>>>> unsigned supports_gmii:1;
>>>> dma_addr_t counters_phys_addr;
>>>> @@ -5091,7 +5093,7 @@ static void rtl_hw_start_8168e_2(struct rtl8169_private *tp)
>>>> RTL_W32(tp, MISC, RTL_R32(tp, MISC) | PWM_EN);
>>>> RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~Spi_en);
>>>>
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8168f(struct rtl8169_private *tp)
>>>> @@ -5205,7 +5207,7 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
>>>> /* disable aspm and clock request before access ephy */
>>>> rtl_hw_aspm_clkreq_enable(tp, false);
>>>> rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1));
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8168g_2(struct rtl8169_private *tp)
>>>> @@ -5240,7 +5242,7 @@ static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
>>>> /* disable aspm and clock request before access ephy */
>>>> rtl_hw_aspm_clkreq_enable(tp, false);
>>>> rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2));
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
>>>> @@ -5337,7 +5339,7 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
>>>> r8168_mac_ocp_write(tp, 0xc094, 0x0000);
>>>> r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
>>>>
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
>>>> @@ -5394,7 +5396,7 @@ static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp)
>>>>
>>>> rtl_hw_start_8168ep(tp);
>>>>
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
>>>> @@ -5414,7 +5416,7 @@ static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
>>>> RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN);
>>>> RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN);
>>>>
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
>>>> @@ -5449,7 +5451,7 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
>>>> data |= 0x0080;
>>>> r8168_mac_ocp_write(tp, 0xe860, data);
>>>>
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8168(struct rtl8169_private *tp)
>>>> @@ -5696,7 +5698,7 @@ static void rtl_hw_start_8106(struct rtl8169_private *tp)
>>>> RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN);
>>>>
>>>> rtl_pcie_state_l2l3_disable(tp);
>>>> - rtl_hw_aspm_clkreq_enable(tp, true);
>>>> + tp->aspm_supported = 1;
>>>> }
>>>>
>>>> static void rtl_hw_start_8101(struct rtl8169_private *tp)
>>>> @@ -7097,6 +7099,49 @@ static const struct rtl_cfg_info {
>>>> }
>>>> };
>>>>
>>>> +static ssize_t rtl_aspm_show(struct device *dev, struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct net_device *ndev = dev_get_drvdata(dev);
>>>> + struct rtl8169_private *tp = netdev_priv(ndev);
>>>> +
>>>> + return snprintf(buf, PAGE_SIZE, "%d\n", tp->aspm_enabled);
>>>> +}
>>>> +
>>>> +static ssize_t rtl_aspm_store(struct device *dev, struct device_attribute *attr,
>>>> + const char *buf, size_t size)
>>>> +{
>>>> + struct net_device *ndev = dev_get_drvdata(dev);
>>>> + struct rtl8169_private *tp = netdev_priv(ndev);
>>>> + bool aspm;
>>>> +
>>>> + if (!tp->aspm_supported)
>>>> + return -EOPNOTSUPP;
>>>> +
>>>> + if (strtobool(buf, &aspm) < 0)
>>>> + return -EINVAL;
>>>> +
>>>> + pm_runtime_get_noresume(dev);
>>>> +
>>>> + if (!pm_runtime_active(dev)) {
>>>> + size = -ENETDOWN;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + rtl_lock_work(tp);
>>>> + rtl_unlock_config_regs(tp);
>>>> + rtl_hw_aspm_clkreq_enable(tp, aspm);
>>>> + tp->aspm_enabled = aspm ? 1 : 0;
>>>> + rtl_lock_config_regs(tp);
>>>> + rtl_unlock_work(tp);
>>>> +out:
>>>> + pm_runtime_put_noidle(dev);
>>>> +
>>>> + return size;
>>>> +}
>>>> +
>>>> +static DEVICE_ATTR_RW(rtl_aspm);
>>>> +
>>>> static int rtl_alloc_irq(struct rtl8169_private *tp)
>>>> {
>>>> unsigned int flags;
>>>> @@ -7325,6 +7370,11 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
>>>> return rc;
>>>> }
>>>>
>>>> +static void rtl_remove_sysfs_aspm_file(void *data)
>>>> +{
>>>> + device_remove_file(data, &dev_attr_rtl_aspm);
>>>> +}
>>>> +
>>>> static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>> {
>>>> const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
>>>> @@ -7498,6 +7548,15 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>
>>>> pci_set_drvdata(pdev, dev);
>>>>
>>>> + rc = device_create_file(&pdev->dev, &dev_attr_rtl_aspm);
>>>> + if (rc)
>>>> + return rc;
>>>> +
>>>> + rc = devm_add_action_or_reset(&pdev->dev, rtl_remove_sysfs_aspm_file,
>>>> + &pdev->dev);
>>>> + if (rc)
>>>> + return rc;
>>>> +
>>>> rc = r8169_mdio_register(tp);
>>>> if (rc)
>>>> return rc;
>>>>
>>>
>>>
>>
>
