linux-kernel - Re: [PATCH] i40e: fix ptp time increment while link is down

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <zmw3whfzcipegeyxpydgctio62q3vlpktddhidu4lskffgr3uk@irzoprznarmd>
Date: Sun, 30 Nov 2025 15:28:35 +0100
From: Markus Blöchl <markus@...chl.de>
To: Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@...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>, Richard Cochran <richardcochran@...il.com>, 
	Markus Blöchl <markus.bloechl@...tronik.com>, intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i40e: fix ptp time increment while link is down

On Tue, Nov 25, 2025 at 02:48:20PM -0800, Tony Nguyen wrote:
> 
> 
> On 11/19/2025 8:09 AM, Markus Blöchl wrote:
> > 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>
> > ---
> 
> ...
> 
> > --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > @@ -847,6 +847,65 @@ 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 the link speed.
> 
> Can you make this 'Return:' to conform with kdoc expectations?
> 
> > + **/
> > +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;
> > +		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;
> 
> I believe operators are supposed to end the line rather than start a new
> one.
> 
> > +		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);
> > +
> > +			prtmac_pcs_linksta = (prtmac_pcs_linksta
> > +			& I40E_PRTMAC_PCS_LINK_STATUS1_LINK_SPEED_MASK)
> > +			>> I40E_PRTMAC_PCS_LINK_STATUS1_LINK_SPEED_SHIFT;
> 
> Same operator comment. Also, indentation looks off here.
> 
> Thanks,
> Tony
> 

Thanks for the close look, Tony.
Will be fixed in v2.
I needed a reason to reroll anyway, since I forgot to base this on
net...

--