netdev - RE: [PATCH net-next 1/4] net: wangxun: Add support for PTP clock

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: <035001db60ab$4707cfd0$d5176f70$@trustnetic.com>
Date: Tue, 7 Jan 2025 10:24:28 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Andrew Lunn'" <andrew@...n.ch>
Cc: <andrew+netdev@...n.ch>,
	<davem@...emloft.net>,
	<edumazet@...gle.com>,
	<kuba@...nel.org>,
	<pabeni@...hat.com>,
	<richardcochran@...il.com>,
	<linux@...linux.org.uk>,
	<horms@...nel.org>,
	<jacob.e.keller@...el.com>,
	<netdev@...r.kernel.org>,
	<mengyuanlou@...-swift.com>
Subject: RE: [PATCH net-next 1/4] net: wangxun: Add support for PTP clock

On Mon, Jan 6, 2025 10:27 PM, Andrew Lunn wrote:
> > > > +	smp_mb(); /* Force any pending update before accessing. */
> > > > +	incval = READ_ONCE(wx->base_incval);
> > > > +	incval = adjust_by_scaled_ppm(incval, ppb);
> > > > +
> > > > +	mask = (wx->mac.type == wx_mac_em) ? 0x7FFFFFF : 0xFFFFFF;
> > > > +	if (incval > mask)
> > > > +		dev_warn(&wx->pdev->dev,
> > > > +			 "PTP ppb adjusted SYSTIME rate overflowed!\n");
> > >
> > > There is no return here, you just keep going. What happens if there is
> > > an overflow?
> >
> > If there is an overflow, the calibration value of this second will be
> > inaccurate. But it does not affect the calibration value of the next
> > second. And this rarely happens.
> 
> If this is a onetime event you don't really care about, is a
> dev_warn() justified? Do you want to be handling the user questions
> about what it means, when all you are going to say is, ignore it, it
> does not really matter?

I'll remove the dev_warn() to avoid user confusion.

> 
> > > > +/**
> > > > + * wx_ptp_tx_hwtstamp_work
> > > > + * @work: pointer to the work struct
> > > > + *
> > > > + * This work item polls TSYNCTXCTL valid bit to determine when a Tx hardware
> > > > + * timestamp has been taken for the current skb. It is necessary, because the
> > > > + * descriptor's "done" bit does not correlate with the timestamp event.
> > > > + */
> > >
> > > Are you saying the "done" bit can be set, but the timestamp is not yet
> > > in place? I've not read the whole patch, but do you start polling once
> > > "done" is set, or as soon at the skbuff is queues for transmission?
> >
> > The descriptor's "done" bit cannot be used as a basis for Tx hardware
> > timestamp. So we should poll the valid bit in the register.
> 
> You did not answer my question. When do you start polling?

As soon at the skbuff is queues for transmission.