| Message ID | 20210819054542.608745-4-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: Implement dynamic ASPM mechanism for recent 1.0/2.5Gbps Realtek NICs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 51 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 19.08.2021 07:45, Kai-Heng Feng wrote: > The latest vendor driver enables ASPM for more recent r8168 NICs, so > disable ASPM on older chips and enable ASPM for the rest. > > Rename aspm_manageable to pcie_aspm_manageable to indicate it's ASPM > from PCIe, and use rtl_aspm_supported for Realtek NIC's internal ASPM > function. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > v3: > - Use pcie_aspm_supported() to retrieve ASPM support status > - Use whitelist for r8169 internal ASPM status > > v2: > - No change > > drivers/net/ethernet/realtek/r8169_main.c | 27 ++++++++++++++++------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 3359509c1c351..88e015d93e490 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -623,7 +623,8 @@ struct rtl8169_private { > } wk; > > unsigned supports_gmii:1; > - unsigned aspm_manageable:1; > + unsigned pcie_aspm_manageable:1; > + unsigned rtl_aspm_supported:1; > unsigned rtl_aspm_enabled:1; > struct delayed_work aspm_toggle; > atomic_t aspm_packet_count; > @@ -702,6 +703,20 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp) > tp->mac_version <= RTL_GIGA_MAC_VER_53; > } > > +static int rtl_supports_aspm(struct rtl8169_private *tp) > +{ > + switch (tp->mac_version) { > + case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_31: > + case RTL_GIGA_MAC_VER_37: > + case RTL_GIGA_MAC_VER_39: > + case RTL_GIGA_MAC_VER_43: > + case RTL_GIGA_MAC_VER_47: > + return 0; > + default: > + return 1; > + } > +} > + > static bool rtl_supports_eee(struct rtl8169_private *tp) > { > return tp->mac_version >= RTL_GIGA_MAC_VER_34 && > @@ -2669,7 +2684,7 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp) > > static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) > { > - if (!tp->aspm_manageable && enable) > + if (!(tp->pcie_aspm_manageable && tp->rtl_aspm_supported) && enable) > return; > > tp->rtl_aspm_enabled = enable; > @@ -5319,12 +5334,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (rc) > return rc; > > - /* Disable ASPM completely as that cause random device stop working > - * problems as well as full system hangs for some PCIe devices users. > - */ > - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | > - PCIE_LINK_STATE_L1); > - tp->aspm_manageable = !rc; > + tp->pcie_aspm_manageable = pcie_aspm_supported(pdev); That's not what I meant, and it's also not correct. > + tp->rtl_aspm_supported = rtl_supports_aspm(tp); > > /* enable device (incl. PCI PM wakeup and hotplug setup) */ > rc = pcim_enable_device(pdev); >
On Thu, Aug 19, 2021 at 2:08 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 19.08.2021 07:45, Kai-Heng Feng wrote: > > The latest vendor driver enables ASPM for more recent r8168 NICs, so > > disable ASPM on older chips and enable ASPM for the rest. > > > > Rename aspm_manageable to pcie_aspm_manageable to indicate it's ASPM > > from PCIe, and use rtl_aspm_supported for Realtek NIC's internal ASPM > > function. > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > v3: > > - Use pcie_aspm_supported() to retrieve ASPM support status > > - Use whitelist for r8169 internal ASPM status > > > > v2: > > - No change > > > > drivers/net/ethernet/realtek/r8169_main.c | 27 ++++++++++++++++------- > > 1 file changed, 19 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > > index 3359509c1c351..88e015d93e490 100644 > > --- a/drivers/net/ethernet/realtek/r8169_main.c > > +++ b/drivers/net/ethernet/realtek/r8169_main.c > > @@ -623,7 +623,8 @@ struct rtl8169_private { > > } wk; > > > > unsigned supports_gmii:1; > > - unsigned aspm_manageable:1; > > + unsigned pcie_aspm_manageable:1; > > + unsigned rtl_aspm_supported:1; > > unsigned rtl_aspm_enabled:1; > > struct delayed_work aspm_toggle; > > atomic_t aspm_packet_count; > > @@ -702,6 +703,20 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp) > > tp->mac_version <= RTL_GIGA_MAC_VER_53; > > } > > > > +static int rtl_supports_aspm(struct rtl8169_private *tp) > > +{ > > + switch (tp->mac_version) { > > + case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_31: > > + case RTL_GIGA_MAC_VER_37: > > + case RTL_GIGA_MAC_VER_39: > > + case RTL_GIGA_MAC_VER_43: > > + case RTL_GIGA_MAC_VER_47: > > + return 0; > > + default: > > + return 1; > > + } > > +} > > + > > static bool rtl_supports_eee(struct rtl8169_private *tp) > > { > > return tp->mac_version >= RTL_GIGA_MAC_VER_34 && > > @@ -2669,7 +2684,7 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp) > > > > static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) > > { > > - if (!tp->aspm_manageable && enable) > > + if (!(tp->pcie_aspm_manageable && tp->rtl_aspm_supported) && enable) > > return; > > > > tp->rtl_aspm_enabled = enable; > > @@ -5319,12 +5334,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > > if (rc) > > return rc; > > > > - /* Disable ASPM completely as that cause random device stop working > > - * problems as well as full system hangs for some PCIe devices users. > > - */ > > - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | > > - PCIE_LINK_STATE_L1); > > - tp->aspm_manageable = !rc; > > + tp->pcie_aspm_manageable = pcie_aspm_supported(pdev); > > That's not what I meant, and it's also not correct. In case I make another mistake in next series, let me ask it more clearly... What you meant was to check both link->aspm_enabled and link->aspm_support? > > > + tp->rtl_aspm_supported = rtl_supports_aspm(tp); Is rtl_supports_aspm() what you expect for the whitelist? And what else am I missing? Kai-Heng > > > > /* enable device (incl. PCI PM wakeup and hotplug setup) */ > > rc = pcim_enable_device(pdev); > > >
On 19.08.2021 08:50, Kai-Heng Feng wrote: > On Thu, Aug 19, 2021 at 2:08 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> On 19.08.2021 07:45, Kai-Heng Feng wrote: >>> The latest vendor driver enables ASPM for more recent r8168 NICs, so >>> disable ASPM on older chips and enable ASPM for the rest. >>> >>> Rename aspm_manageable to pcie_aspm_manageable to indicate it's ASPM >>> from PCIe, and use rtl_aspm_supported for Realtek NIC's internal ASPM >>> function. >>> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>> --- >>> v3: >>> - Use pcie_aspm_supported() to retrieve ASPM support status >>> - Use whitelist for r8169 internal ASPM status >>> >>> v2: >>> - No change >>> >>> drivers/net/ethernet/realtek/r8169_main.c | 27 ++++++++++++++++------- >>> 1 file changed, 19 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >>> index 3359509c1c351..88e015d93e490 100644 >>> --- a/drivers/net/ethernet/realtek/r8169_main.c >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c >>> @@ -623,7 +623,8 @@ struct rtl8169_private { >>> } wk; >>> >>> unsigned supports_gmii:1; >>> - unsigned aspm_manageable:1; >>> + unsigned pcie_aspm_manageable:1; >>> + unsigned rtl_aspm_supported:1; >>> unsigned rtl_aspm_enabled:1; >>> struct delayed_work aspm_toggle; >>> atomic_t aspm_packet_count; >>> @@ -702,6 +703,20 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp) >>> tp->mac_version <= RTL_GIGA_MAC_VER_53; >>> } >>> >>> +static int rtl_supports_aspm(struct rtl8169_private *tp) >>> +{ >>> + switch (tp->mac_version) { >>> + case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_31: >>> + case RTL_GIGA_MAC_VER_37: >>> + case RTL_GIGA_MAC_VER_39: >>> + case RTL_GIGA_MAC_VER_43: >>> + case RTL_GIGA_MAC_VER_47: >>> + return 0; >>> + default: >>> + return 1; >>> + } >>> +} >>> + >>> static bool rtl_supports_eee(struct rtl8169_private *tp) >>> { >>> return tp->mac_version >= RTL_GIGA_MAC_VER_34 && >>> @@ -2669,7 +2684,7 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp) >>> >>> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) >>> { >>> - if (!tp->aspm_manageable && enable) >>> + if (!(tp->pcie_aspm_manageable && tp->rtl_aspm_supported) && enable) >>> return; >>> >>> tp->rtl_aspm_enabled = enable; >>> @@ -5319,12 +5334,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >>> if (rc) >>> return rc; >>> >>> - /* Disable ASPM completely as that cause random device stop working >>> - * problems as well as full system hangs for some PCIe devices users. >>> - */ >>> - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | >>> - PCIE_LINK_STATE_L1); >>> - tp->aspm_manageable = !rc; >>> + tp->pcie_aspm_manageable = pcie_aspm_supported(pdev); >> >> That's not what I meant, and it's also not correct. > > In case I make another mistake in next series, let me ask it more clearly... > What you meant was to check both link->aspm_enabled and link->aspm_support? > aspm_enabled can be changed by the user at any time. pci_disable_link_state() also considers whether BIOS forbids that OS mess with ASPM. See aspm_disabled. >> >>> + tp->rtl_aspm_supported = rtl_supports_aspm(tp); > > Is rtl_supports_aspm() what you expect for the whitelist? > And what else am I missing? > I meant use rtl_supports_aspm() to check when ASPM is relevant at all, and in addition use a blacklist for chip versions where ASPM is completely unusable. > Kai-Heng > >>> >>> /* enable device (incl. PCI PM wakeup and hotplug setup) */ >>> rc = pcim_enable_device(pdev); >>> >>
On Thu, Aug 19, 2021 at 5:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 19.08.2021 08:50, Kai-Heng Feng wrote: > > On Thu, Aug 19, 2021 at 2:08 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> On 19.08.2021 07:45, Kai-Heng Feng wrote: > >>> The latest vendor driver enables ASPM for more recent r8168 NICs, so > >>> disable ASPM on older chips and enable ASPM for the rest. > >>> > >>> Rename aspm_manageable to pcie_aspm_manageable to indicate it's ASPM > >>> from PCIe, and use rtl_aspm_supported for Realtek NIC's internal ASPM > >>> function. > >>> > >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >>> --- > >>> v3: > >>> - Use pcie_aspm_supported() to retrieve ASPM support status > >>> - Use whitelist for r8169 internal ASPM status > >>> > >>> v2: > >>> - No change > >>> > >>> drivers/net/ethernet/realtek/r8169_main.c | 27 ++++++++++++++++------- > >>> 1 file changed, 19 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > >>> index 3359509c1c351..88e015d93e490 100644 > >>> --- a/drivers/net/ethernet/realtek/r8169_main.c > >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >>> @@ -623,7 +623,8 @@ struct rtl8169_private { > >>> } wk; > >>> > >>> unsigned supports_gmii:1; > >>> - unsigned aspm_manageable:1; > >>> + unsigned pcie_aspm_manageable:1; > >>> + unsigned rtl_aspm_supported:1; > >>> unsigned rtl_aspm_enabled:1; > >>> struct delayed_work aspm_toggle; > >>> atomic_t aspm_packet_count; > >>> @@ -702,6 +703,20 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp) > >>> tp->mac_version <= RTL_GIGA_MAC_VER_53; > >>> } > >>> > >>> +static int rtl_supports_aspm(struct rtl8169_private *tp) > >>> +{ > >>> + switch (tp->mac_version) { > >>> + case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_31: > >>> + case RTL_GIGA_MAC_VER_37: > >>> + case RTL_GIGA_MAC_VER_39: > >>> + case RTL_GIGA_MAC_VER_43: > >>> + case RTL_GIGA_MAC_VER_47: > >>> + return 0; > >>> + default: > >>> + return 1; > >>> + } > >>> +} > >>> + > >>> static bool rtl_supports_eee(struct rtl8169_private *tp) > >>> { > >>> return tp->mac_version >= RTL_GIGA_MAC_VER_34 && > >>> @@ -2669,7 +2684,7 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp) > >>> > >>> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) > >>> { > >>> - if (!tp->aspm_manageable && enable) > >>> + if (!(tp->pcie_aspm_manageable && tp->rtl_aspm_supported) && enable) > >>> return; > >>> > >>> tp->rtl_aspm_enabled = enable; > >>> @@ -5319,12 +5334,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > >>> if (rc) > >>> return rc; > >>> > >>> - /* Disable ASPM completely as that cause random device stop working > >>> - * problems as well as full system hangs for some PCIe devices users. > >>> - */ > >>> - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | > >>> - PCIE_LINK_STATE_L1); > >>> - tp->aspm_manageable = !rc; > >>> + tp->pcie_aspm_manageable = pcie_aspm_supported(pdev); > >> > >> That's not what I meant, and it's also not correct. > > > > In case I make another mistake in next series, let me ask it more clearly... > > What you meant was to check both link->aspm_enabled and link->aspm_support? > > > aspm_enabled can be changed by the user at any time. OK, will check that too. > pci_disable_link_state() also considers whether BIOS forbids that OS > mess with ASPM. See aspm_disabled. I think aspm_disabled means leave BIOS ASPM setting intact? So If PCIe ASPM is already enabled, we should also enable Realtek specific bits to make ASPM really work. > > >> > >>> + tp->rtl_aspm_supported = rtl_supports_aspm(tp); > > > > Is rtl_supports_aspm() what you expect for the whitelist? > > And what else am I missing? > > > I meant use rtl_supports_aspm() to check when ASPM is relevant at all, I think that means the relevant bits are link->aspm_capable and pcie_aspm_support_enabled(). ASPM can be already enabled by BIOS with aspm_disabled set. Then check link->aspm_enabled in aspm_toggle() routine because it can be enabled at runtime. > and in addition use a blacklist for chip versions where ASPM is > completely unusable. Thanks for your suggestion and review. Kai-Heng > > > Kai-Heng > > > >>> > >>> /* enable device (incl. PCI PM wakeup and hotplug setup) */ > >>> rc = pcim_enable_device(pdev); > >>> > >> >
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 3359509c1c351..88e015d93e490 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -623,7 +623,8 @@ struct rtl8169_private { } wk; unsigned supports_gmii:1; - unsigned aspm_manageable:1; + unsigned pcie_aspm_manageable:1; + unsigned rtl_aspm_supported:1; unsigned rtl_aspm_enabled:1; struct delayed_work aspm_toggle; atomic_t aspm_packet_count; @@ -702,6 +703,20 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp) tp->mac_version <= RTL_GIGA_MAC_VER_53; } +static int rtl_supports_aspm(struct rtl8169_private *tp) +{ + switch (tp->mac_version) { + case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_31: + case RTL_GIGA_MAC_VER_37: + case RTL_GIGA_MAC_VER_39: + case RTL_GIGA_MAC_VER_43: + case RTL_GIGA_MAC_VER_47: + return 0; + default: + return 1; + } +} + static bool rtl_supports_eee(struct rtl8169_private *tp) { return tp->mac_version >= RTL_GIGA_MAC_VER_34 && @@ -2669,7 +2684,7 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp) static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) { - if (!tp->aspm_manageable && enable) + if (!(tp->pcie_aspm_manageable && tp->rtl_aspm_supported) && enable) return; tp->rtl_aspm_enabled = enable; @@ -5319,12 +5334,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) if (rc) return rc; - /* Disable ASPM completely as that cause random device stop working - * problems as well as full system hangs for some PCIe devices users. - */ - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | - PCIE_LINK_STATE_L1); - tp->aspm_manageable = !rc; + tp->pcie_aspm_manageable = pcie_aspm_supported(pdev); + tp->rtl_aspm_supported = rtl_supports_aspm(tp); /* enable device (incl. PCI PM wakeup and hotplug setup) */ rc = pcim_enable_device(pdev);
The latest vendor driver enables ASPM for more recent r8168 NICs, so disable ASPM on older chips and enable ASPM for the rest. Rename aspm_manageable to pcie_aspm_manageable to indicate it's ASPM from PCIe, and use rtl_aspm_supported for Realtek NIC's internal ASPM function. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v3: - Use pcie_aspm_supported() to retrieve ASPM support status - Use whitelist for r8169 internal ASPM status v2: - No change drivers/net/ethernet/realtek/r8169_main.c | 27 ++++++++++++++++------- 1 file changed, 19 insertions(+), 8 deletions(-)