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 for Android: free password hash cracker in your pocket

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

Message-ID: <a4576efa-2d20-47e9-b785-66dbfda78633@lunn.ch>
Date: Mon, 6 Jan 2025 15:26:55 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
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

> > > +	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?

> > > +/**
> > > + * 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?

	Andrew