| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4f1486d2bf5248809f943d4c22fbd521@realtek.com>
Date: Thu, 6 Mar 2025 16:51:20 +0000
From: Hau <hau@...ltek.com>
To: Heiner Kallweit <hkallweit1@...il.com>, Bjorn Helgaas <helgaas@...nel.org>
CC: nic_swsd <nic_swsd@...ltek.com>,
"andrew+netdev@...n.ch"
<andrew+netdev@...n.ch>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org"
<kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
Bjorn Helgaas
<bhelgaas@...gle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: RE: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@...il.com]
> Sent: Saturday, March 1, 2025 6:58 AM
> To: Bjorn Helgaas <helgaas@...nel.org>; Hau <hau@...ltek.com>
> Cc: nic_swsd <nic_swsd@...ltek.com>; andrew+netdev@...n.ch;
> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; Bjorn Helgaas <bhelgaas@...gle.com>;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; linux-
> pci@...r.kernel.org
> Subject: Re: [PATCH net-next 2/3] r8169: enable
> RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
>
>
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
>
>
>
> On 24.02.2025 20:00, Bjorn Helgaas wrote:
> > On Mon, Feb 24, 2025 at 04:33:50PM +0000, Hau wrote:
> >>> On 21.02.2025 08:18, ChunHao Lin wrote:
> >>>> This patch will enable
> RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126
> >>>> LTR support on the platforms that have tested with LTR enabled.
> >>>
> >>> Where in the code is the check whether platform has been tested with
> LTR?
> >>>
> >> LTR is for L1,2. But L1 will be disabled when rtl_aspm_is_safe()
> >> return false. So LTR needs rtl_aspm_is_safe() to return true.
> >>
> >>>> Signed-off-by: ChunHao Lin <hau@...ltek.com>
> >>>> ---
> >>>> drivers/net/ethernet/realtek/r8169_main.c | 108
> >>>> ++++++++++++++++++++++
> >>>> 1 file changed, 108 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> >>>> b/drivers/net/ethernet/realtek/r8169_main.c
> >>>> index 731302361989..9953eaa01c9d 100644
> >>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>>> @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct
> >>> rtl8169_private *tp)
> >>>> }
> >>>> }
> >>>>
> >>>> +static void rtl_set_ltr_latency(struct rtl8169_private *tp) {
> >>>> + switch (tp->mac_version) {
> >>>> + case RTL_GIGA_MAC_VER_70:
> >>>> + case RTL_GIGA_MAC_VER_71:
> >>>> + r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
> >>>> + r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
> >>>> + r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
> >>>> + r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
> >>>> + r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
> >>>> + break;
> >>>> + case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
> >>>> + r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
> >>>> + r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
> >>>> + r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
> >>>> + r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
> >>>> + r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> >>>> + break;
> >>>> + case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
> >>>> + r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
> >>>> + r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> >>>> + break;
> >>>> + default:
> >>>> + break;
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +static void rtl_reset_pci_ltr(struct rtl8169_private *tp) {
> >>>> + struct pci_dev *pdev = tp->pci_dev;
> >>>> + u16 cap;
> >>>> +
> >>>> + pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
> >>>> + if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
> >>>> + pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> >>>> + PCI_EXP_DEVCTL2_LTR_EN);
> >>>> + pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> >>>> + PCI_EXP_DEVCTL2_LTR_EN);
> >>>
> >>> I'd prefer that only PCI core deals with these registers (functions
> >>> like pci_configure_ltr()). Any specific reason for this reset? Is it
> >>> something which could be applicable for other devices too, so that
> >>> the PCI core should be extended?
> >>>
> >> It is for specific platform. On that platform driver needs to do this
> >> to let LTR works.
> >
> I interpret this in a way that the chip triggers some internal LTR configuration
> activity if it detects bit PCI_EXP_DEVCTL2_LTR_EN changing from 0 to 1. And
> this needed activity isn't triggered if PCI_EXP_DEVCTL2_LTR_EN is set already
> and doesn't change.
> Hau, is this correct?
>
It may be the reason for the LTR of these platforms to work. But I will remove this
quirk from next version patch.
> So the PCI_EXP_DEVCTL2_LTR_EN reset is some kind of needed quirk.
> However PCI quirks are applied too early, before we even detected the chip
> version in probe(). Therefore I also think a helper for this reset in PCI core
> would be best.
>
> And what hasn't been mentioned yet: We have to skip the chip-specific LTR
> configuration if pci_dev->ltr_path isn't set.
>
I will add checking pci_dev->ltr_path in next version patch.
> > This definitely looks like code that should not be in a driver.
> > Drivers shouldn't need to touch ASPM or LTR configuration unless
> > there's a device defect to work around, and that should use a PCI core
> > interface. Depending on what the defect is, we may need to add a new
> > interface.
> >
> > This clear/set of PCI_EXP_DEVCTL2_LTR_EN when it was already set could
> > work around some kind of device defect, or it could be a hint that
> > something in the PCI core is broken. Maybe the core is configuring
> > ASPM/LTR incorrectly.
> >
> > Bjorn
Powered by blists - more mailing lists