| 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: <IA3PR11MB898686B5873D85A56CDE9CECE5ADA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Mon, 15 Dec 2025 07:14:08 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Markus Blöchl <markus@...chl.de>, "Nguyen, Anthony L"
<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, "Keller, Jacob E" <jacob.e.keller@...el.com>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, "Paolo
Abeni" <pabeni@...hat.com>
CC: Richard Cochran <richardcochran@...il.com>, "Bloechl, Markus"
<markus.bloechl@...tronik.com>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH net v2] i40e: fix ptp time increment
while link is down
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf
> Of Markus Blöchl
> Sent: Sunday, November 30, 2025 4:24 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@...el.com>; Keller, Jacob E
> <jacob.e.keller@...el.com>; Andrew Lunn <andrew+netdev@...n.ch>; David
> S. Miller <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>;
> Jakub Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>
> Cc: Richard Cochran <richardcochran@...il.com>; Bloechl, Markus
> <markus.bloechl@...tronik.com>; intel-wired-lan@...ts.osuosl.org;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Markus Blöchl
> <markus@...chl.de>
> Subject: [Intel-wired-lan] [PATCH net v2] i40e: fix ptp time increment
> while link is down
>
> When an X710 ethernet port with an active ptp daemon (like the ptp4l
> and phc2sys combo) suddenly loses its link and regains it after a
> while, the ptp daemon has a hard time to recover synchronization and
> sometimes entirely fails to do so.
>
> The issue seems to be related to a wrongly configured increment while
> the link is down. This could not be observed with the Intel reference
> driver.
> We identified the fix to appear in Intels official ethernet-linux-i40e
> release version 2.17.4.
>
> Include the relevant changes in the kernel version of this driver.
>
> Fixes: beb0dff1251d ("i40e: enable PTP")
> Cc: stable@...r.kernel.org
> Signed-off-by: Markus Blöchl <markus@...chl.de>
> ---
> Tested with an X710 at 10G link speed and kernel version 6.12.42.
> ---
> Changes in v2:
> - Fix kdoc and code formatting
> - Rebase onto net tree
> - Link to v1: https://lore.kernel.org/r/20251119-i40e_ptp_link_down-
> v1-1-b351fed254b3@...chl.de
> ---
> drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h | 9 +++
> drivers/net/ethernet/intel/i40e/i40e_ptp.c | 69
> +++++++++++++++++++++--
> drivers/net/ethernet/intel/i40e/i40e_register.h | 9 +++
> drivers/net/ethernet/intel/i40e/i40e_type.h | 8 +++
> 4 files changed, 90 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> index cc02a85ad42b..ec176e9569ad 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> @@ -1488,6 +1488,15 @@ enum i40e_aq_link_speed {
> I40E_LINK_SPEED_25GB = BIT(I40E_LINK_SPEED_25GB_SHIFT),
> };
>
> +enum i40e_prt_mac_pcs_link_speed {
> + I40E_PRT_MAC_PCS_LINK_SPEED_UNKNOWN = 0,
> + I40E_PRT_MAC_PCS_LINK_SPEED_100MB,
> + I40E_PRT_MAC_PCS_LINK_SPEED_1GB,
> + I40E_PRT_MAC_PCS_LINK_SPEED_10GB,
> + I40E_PRT_MAC_PCS_LINK_SPEED_40GB,
> + I40E_PRT_MAC_PCS_LINK_SPEED_20GB
> +};
> +
> struct i40e_aqc_module_desc {
> u8 oui[3];
> u8 reserved1;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> index 33535418178b..89abe2f22216 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> @@ -847,6 +847,66 @@ void i40e_ptp_rx_hwtstamp(struct i40e_pf *pf,
> struct sk_buff *skb, u8 index)
> i40e_ptp_convert_to_hwtstamp(skb_hwtstamps(skb), ns); }
>
> +/**
> + * i40e_ptp_get_link_speed_hw - get the link speed
> + * @pf: Board private structure
> + *
> + * Calculate link speed depending on the link status.
> + *
> + * Return: current link speed.
> + **/
The function uses hardcoded port index 0 in register accesses (I40E_PRTMAC_LINKSTA(0), I40E_PRTMAC_PCS_LINK_STATUS1(0)),
but this limitation is not documented. Additionally, the return value documentation is vague.
> +static enum i40e_aq_link_speed i40e_ptp_get_link_speed_hw(struct
> +i40e_pf *pf) {
> + bool link_up = pf->hw.phy.link_info.link_info &
> I40E_AQ_LINK_UP;
> + enum i40e_aq_link_speed link_speed = I40E_LINK_SPEED_UNKNOWN;
> + struct i40e_hw *hw = &pf->hw;
> +
> + if (link_up) {
> + struct i40e_link_status *hw_link_info = &hw-
> >phy.link_info;
> +
> + i40e_aq_get_link_info(hw, true, NULL, NULL);
> + link_speed = hw_link_info->link_speed;
> + } else {
> + enum i40e_prt_mac_link_speed prtmac_linksta;
I think Linux kernel coding style discourages implicit conversions between enum types and integer types without explicit cast.
Better declare u32 prtmac_linksta;
> + u64 prtmac_pcs_linksta;
> +
> + prtmac_linksta = (rd32(hw, I40E_PRTMAC_LINKSTA(0)) &
> + I40E_PRTMAC_LINKSTA_MAC_LINK_SPEED_MASK)
> >>
> + I40E_PRTMAC_LINKSTA_MAC_LINK_SPEED_SHIFT;
> + if (prtmac_linksta == I40E_PRT_MAC_LINK_SPEED_40GB) {
> + link_speed = I40E_LINK_SPEED_40GB;
> + } else {
> + i40e_aq_debug_read_register(hw,
> +
> I40E_PRTMAC_PCS_LINK_STATUS1(0),
> + &prtmac_pcs_linksta,
> + NULL);
Both i40e_aq_get_link_info() and i40e_aq_debug_read_register() return error status codes (i40e_status_code / int),
but these return values are completely ignored. If either admin queue command fails,
the function proceeds with potentially stale or uninitialized data.
> +
> + prtmac_pcs_linksta = (prtmac_pcs_linksta &
> +
> I40E_PRTMAC_PCS_LINK_STATUS1_LINK_SPEED_MASK) >>
> +
> I40E_PRTMAC_PCS_LINK_STATUS1_LINK_SPEED_SHIFT;
> +
> + switch (prtmac_pcs_linksta) {
> + case I40E_PRT_MAC_PCS_LINK_SPEED_100MB:
> + link_speed = I40E_LINK_SPEED_100MB;
> + break;
> + case I40E_PRT_MAC_PCS_LINK_SPEED_1GB:
> + link_speed = I40E_LINK_SPEED_1GB;
> + break;
> + case I40E_PRT_MAC_PCS_LINK_SPEED_10GB:
> + link_speed = I40E_LINK_SPEED_10GB;
> + break;
> + case I40E_PRT_MAC_PCS_LINK_SPEED_20GB:
> + link_speed = I40E_LINK_SPEED_20GB;
> + break;
> + default:
> + link_speed = I40E_LINK_SPEED_UNKNOWN;
> + }
> + }
> + }
> +
> + return link_speed;
> +}
> +
> /**
> * i40e_ptp_set_increment - Utility function to update clock
> increment rate
> * @pf: Board private structure
> @@ -857,16 +917,14 @@ void i40e_ptp_rx_hwtstamp(struct i40e_pf *pf,
> struct sk_buff *skb, u8 index)
> **/
> void i40e_ptp_set_increment(struct i40e_pf *pf) {
> - struct i40e_link_status *hw_link_info;
> + enum i40e_aq_link_speed link_speed;
> struct i40e_hw *hw = &pf->hw;
> u64 incval;
> u32 mult;
>
> - hw_link_info = &hw->phy.link_info;
> + link_speed = i40e_ptp_get_link_speed_hw(pf);
>
> - i40e_aq_get_link_info(&pf->hw, true, NULL, NULL);
> -
> - switch (hw_link_info->link_speed) {
> + switch (link_speed) {
> case I40E_LINK_SPEED_10GB:
> mult = I40E_PTP_10GB_INCVAL_MULT;
> break;
> @@ -909,6 +967,7 @@ void i40e_ptp_set_increment(struct i40e_pf *pf)
> /* Update the base adjustement value. */
> WRITE_ONCE(pf->ptp_adj_mult, mult);
> smp_mb(); /* Force the above update. */
> + i40e_ptp_set_1pps_signal_hw(pf);
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_register.h
> b/drivers/net/ethernet/intel/i40e/i40e_register.h
> index 432afbb64201..c4051dbcc297 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_register.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_register.h
> @@ -530,6 +530,15 @@
> #define I40E_PRTMAC_HSEC_CTL_TX_PAUSE_REFRESH_TIMER_SHIFT 0 #define
> I40E_PRTMAC_HSEC_CTL_TX_PAUSE_REFRESH_TIMER_MASK I40E_MASK(0xFFFF, \
> I40E_PRTMAC_HSEC_CTL_TX_PAUSE_REFRESH_TIMER_SHIFT)
> +/* _i=0...3 */ /* Reset: GLOBR */
> +#define I40E_PRTMAC_PCS_LINK_STATUS1(_i) (0x0008C200 + ((_i) * 4))
> +#define I40E_PRTMAC_PCS_LINK_STATUS1_LINK_SPEED_SHIFT 24 #define
> +I40E_PRTMAC_PCS_LINK_STATUS1_LINK_SPEED_MASK I40E_MASK(0x7,
> +I40E_PRTMAC_PCS_LINK_STATUS1_LINK_SPEED_SHIFT)
> +#define I40E_PRTMAC_PCS_LINK_STATUS2 0x0008C220
> +/* _i=0...3 */ /* Reset: GLOBR */
> +#define I40E_PRTMAC_LINKSTA(_i) (0x001E2420 + ((_i) * 4)) #define
> +I40E_PRTMAC_LINKSTA_MAC_LINK_SPEED_SHIFT 27 #define
> +I40E_PRTMAC_LINKSTA_MAC_LINK_SPEED_MASK I40E_MASK(0x7,
> +I40E_PRTMAC_LINKSTA_MAC_LINK_SPEED_SHIFT)
> #define I40E_GLNVM_FLA 0x000B6108 /* Reset: POR */ #define
> I40E_GLNVM_FLA_LOCKED_SHIFT 6 #define I40E_GLNVM_FLA_LOCKED_MASK
> I40E_MASK(0x1, I40E_GLNVM_FLA_LOCKED_SHIFT) diff --git
> a/drivers/net/ethernet/intel/i40e/i40e_type.h
> b/drivers/net/ethernet/intel/i40e/i40e_type.h
> index ed8bbdb586da..98c8c5709e5f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_type.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
> @@ -115,6 +115,14 @@ enum i40e_queue_type {
> I40E_QUEUE_TYPE_UNKNOWN
> };
>
> +enum i40e_prt_mac_link_speed {
> + I40E_PRT_MAC_LINK_SPEED_100MB = 0,
> + I40E_PRT_MAC_LINK_SPEED_1GB,
> + I40E_PRT_MAC_LINK_SPEED_10GB,
> + I40E_PRT_MAC_LINK_SPEED_40GB,
> + I40E_PRT_MAC_LINK_SPEED_20GB
> +};
> +
> struct i40e_link_status {
> enum i40e_aq_phy_type phy_type;
> enum i40e_aq_link_speed link_speed;
>
> ---
> base-commit: e5235eb6cfe02a51256013a78f7b28779a7740d5
> change-id: 20251119-i40e_ptp_link_down-47934f9e155d
>
> Best regards,
> --
> Markus Blöchl <markus@...chl.de>
>
>
> --
With the best regards
Alex
Powered by blists - more mailing lists