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: <032b01db600e$8d845430$a88cfc90$@trustnetic.com>
Date: Mon, 6 Jan 2025 15:42:35 +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 Thu, Jan 2, 2025 10:13 PM, Andrew Lunn wrote:
> > +static int wx_tx_map(struct wx_ring *tx_ring,
> > +		     struct wx_tx_buffer *first,
> > +		     const u8 hdr_len)
> >  {
> >  	struct sk_buff *skb = first->skb;
> >  	struct wx_tx_buffer *tx_buffer;
> > @@ -1013,6 +1023,8 @@ static void wx_tx_map(struct wx_ring *tx_ring,
> >
> >  	netdev_tx_sent_queue(wx_txring_txq(tx_ring), first->bytecount);
> >
> > +	/* set the timestamp */
> > +	first->time_stamp = jiffies;
> >  	skb_tx_timestamp(skb);
> >
> >  	/* Force memory writes to complete before letting h/w know there
> > @@ -1038,7 +1050,7 @@ static void wx_tx_map(struct wx_ring *tx_ring,
> >  	if (netif_xmit_stopped(wx_txring_txq(tx_ring)) || !netdev_xmit_more())
> >  		writel(i, tx_ring->tail);
> >
> > -	return;
> > +	return 0;
> >  dma_error:
> >  	dev_err(tx_ring->dev, "TX DMA map failed\n");
> >
> > @@ -1062,6 +1074,8 @@ static void wx_tx_map(struct wx_ring *tx_ring,
> >  	first->skb = NULL;
> >
> >  	tx_ring->next_to_use = i;
> > +
> > +	return -EPERM;
> 
>        EPERM           Operation not permitted (POSIX.1-2001).
> 
> This is normally about restricted access because of security
> settings. So i don't think this is the correct error code here. What
> is the reason the function is exiting with an error? Once we
> understand that, maybe we can suggest a better error code.

I'll change it to -ENOMEM.

> 
> > +static int wx_ptp_adjfine(struct ptp_clock_info *ptp, long ppb)
> > +{
> > +	struct wx *wx = container_of(ptp, struct wx, ptp_caps);
> > +	u64 incval, mask;
> > +
> > +	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.

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

> 
> >  static void ngbe_mac_link_down(struct phylink_config *config,
> >  			       unsigned int mode, phy_interface_t interface)
> >  {
> > +	struct wx *wx = phylink_to_wx(config);
> > +
> > +	wx->speed = SPEED_UNKNOWN;
> > +	if (test_bit(WX_STATE_PTP_RUNNING, wx->state))
> > +		wx_ptp_start_cyclecounter(wx);
> 
> This is probably a naming issue, but it seems odd to call a _start_
> function on link down.

I think this function could be named wx_ptp_reset_cyclecounter().