linux-kernel - RE: [PATCH net-next 02/12] ptp: netc: add NETC Timer PTP driver support

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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <PAXPR04MB85103C39930C9F8DC9453DCA8854A@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Mon, 14 Jul 2025 02:57:45 +0000
From: Wei Fang <wei.fang@....com>
To: Vadim Fedorenko <vfedorenko@...ek.ru>
CC: "F.S. Peng" <fushi.peng@....com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"richardcochran@...il.com" <richardcochran@...il.com>, Claudiu Manoil
	<claudiu.manoil@....com>, Vladimir Oltean <vladimir.oltean@....com>, Clark
 Wang <xiaoning.wang@....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>
Subject: RE: [PATCH net-next 02/12] ptp: netc: add NETC Timer PTP driver
 support

> > +static void netc_timer_offset_write(struct netc_timer *priv, u64 offset)
> > +{
> > +	u32 tmr_off_h = upper_32_bits(offset);
> > +	u32 tmr_off_l = lower_32_bits(offset);
> > +
> > +	netc_timer_wr(priv, NETC_TMR_OFF_L, tmr_off_l);
> > +	netc_timer_wr(priv, NETC_TMR_OFF_H, tmr_off_h);
> > +}
> > +
> > +static u64 netc_timer_cur_time_read(struct netc_timer *priv)
> > +{
> > +	u32 time_h, time_l;
> > +	u64 ns;
> > +
> > +	time_l = netc_timer_rd(priv, NETC_TMR_CUR_TIME_L);
> > +	time_h = netc_timer_rd(priv, NETC_TMR_CUR_TIME_H);
> > +	ns = (u64)time_h << 32 | time_l;
> 
> I assume that the high part is latched after reading low part, but would like
> you confirm it and put a comment as you did for counter read.
> 

Yes, for the current implementation, we should reads from the TMR_CNT_L
register copy the entire 64-bit clock time into the TMR_CNT_H/L shadow
registers. I will add a comment here, thanks.

> > +
> > +	return ns;
> > +}
> > +
> > +static void netc_timer_alarm_write(struct netc_timer *priv,
> > +				   u64 alarm, int index)
> > +{
> > +	u32 alarm_h = upper_32_bits(alarm);
> > +	u32 alarm_l = lower_32_bits(alarm);
> > +
> > +	netc_timer_wr(priv, NETC_TMR_ALARM_L(index), alarm_l);
> > +	netc_timer_wr(priv, NETC_TMR_ALARM_H(index), alarm_h);
> > +}
> > +
> > +static void netc_timer_adjust_period(struct netc_timer *priv, u64 period)
> > +{
> > +	u32 fractional_period = lower_32_bits(period);
> > +	u32 integral_period = upper_32_bits(period);
> > +	u32 tmr_ctrl, old_tmr_ctrl;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +
> > +	old_tmr_ctrl = netc_timer_rd(priv, NETC_TMR_CTRL);
> > +	tmr_ctrl = u32_replace_bits(old_tmr_ctrl, integral_period,
> > +				    TMR_CTRL_TCLK_PERIOD);
> > +	if (tmr_ctrl != old_tmr_ctrl)
> > +		netc_timer_wr(priv, NETC_TMR_CTRL, tmr_ctrl);
> > +
> > +	netc_timer_wr(priv, NETC_TMR_ADD, fractional_period);
> > +
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +}
> > +
> > +static int netc_timer_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> > +{
> > +	struct netc_timer *priv = ptp_to_netc_timer(ptp);
> > +	u64 new_period;
> > +
> > +	if (!scaled_ppm)
> > +		return 0;
> 
> why do you ignore value of 0 here? the adjustment will not happen if for some
> reasons the offset will be aligned after previous adjustment. And there will be
> inconsistency between hardware value and the last stored value in software.
> 

You're right, I wasn't aware of that, thank you.

> > +
> > +	new_period = adjust_by_scaled_ppm(priv->period, scaled_ppm);
> > +	netc_timer_adjust_period(priv, new_period);
> > +
> > +	return 0;
> > +}
> > +
> > +int netc_timer_get_phc_index(struct pci_dev *timer_pdev)
> > +{
> > +	struct netc_timer *priv;
> > +
> > +	if (!timer_pdev)
> > +		return -ENODEV;
> 
> I'm not sure, but looks like this should never happen. Could you please explain
> what are protecting from? If it's just safety, then it's better to remove the
> check and let the kernel crash to figure out wrong usage.

This function is exported for enetc driver to use, and the enetc supports SR-IOV,
if the VF driver and Timer driver run in the same OS, actually, the VF driver can
also support PTP synchronization. But if the VF driver runs in a guest OS, then the
VF driver cannot get the PCIe device of Timer, so the pointer will be NULL. This is
one of the use cases, there are other use cases that Timer is not available, so we
put this check inside this function.

> > +
> > +	priv = pci_get_drvdata(timer_pdev);
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	return priv->phc_index;
> > +}