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: <ab140807-2e49-4782-a58c-2b8d60da5556@lunn.ch>
Date: Thu, 2 Jan 2025 15:13:05 +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

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

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

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

>  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. 

	Andrew