netdev - Re: [PATCH net-next] net: micrel: Change to receive timestamp in the frame for lan8841

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: <ZIDCpPbCFCxKBV2k@hoboy.vegasvil.org>
Date: Wed, 7 Jun 2023 10:47:16 -0700
From: Richard Cochran <richardcochran@...il.com>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, andrew@...n.ch,
	hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next] net: micrel: Change to receive timestamp in the
 frame for lan8841

On Wed, Jun 07, 2023 at 09:09:48AM +0200, Horatiu Vultur wrote:

> Doing these changes to start to get the received timestamp in the
> reserved field of the header, will give a great CPU usage performance.
> Running ptp4l with logSyncInterval of -9 will give a ~50% CPU
> improvment.

Really?

> -static struct lan8814_ptp_rx_ts *lan8841_ptp_get_rx_ts(struct kszphy_ptp_priv *ptp_priv)
> -{
> -	struct phy_device *phydev = ptp_priv->phydev;
> -	struct lan8814_ptp_rx_ts *rx_ts;
> -	u32 sec, nsec;
> -	u16 seq;
> -
> -	nsec = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_NS_HI);
> -	if (!(nsec & LAN8841_PTP_RX_INGRESS_NSEC_HI_VALID))
> -		return NULL;
> -
> -	nsec = ((nsec & 0x3fff) << 16);
> -	nsec = nsec | phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_NS_LO);
> -
> -	sec = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_SEC_HI);
> -	sec = sec << 16;
> -	sec = sec | phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_SEC_LO);
> -
> -	seq = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_MSG_HEADER2);

Before: 5x phy_read_mmd() per frame ...

> -	rx_ts = kzalloc(sizeof(*rx_ts), GFP_KERNEL);
> -	if (!rx_ts)
> -		return NULL;
> -
> -	rx_ts->seconds = sec;
> -	rx_ts->nsec = nsec;
> -	rx_ts->seq_id = seq;
> -
> -	return rx_ts;
> -}

> +static void lan8841_ptp_getseconds(struct ptp_clock_info *ptp,
> +				   struct timespec64 *ts)
> +{
> +	struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> +							ptp_clock_info);
> +	struct phy_device *phydev = ptp_priv->phydev;
> +	time64_t s;
> +
> +	mutex_lock(&ptp_priv->ptp_lock);
> +	/* Issue the command to read the LTC */
> +	phy_write_mmd(phydev, 2, LAN8841_PTP_CMD_CTL,
> +		      LAN8841_PTP_CMD_CTL_PTP_LTC_READ);
> +
> +	/* Read the LTC */
> +	s = phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_HI);
> +	s <<= 16;
> +	s |= phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_MID);
> +	s <<= 16;
> +	s |= phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_LO);

After: 4x phy_read_mmd() per frame.  How does that save 50% cpu?

> +	mutex_unlock(&ptp_priv->ptp_lock);
> +
> +	set_normalized_timespec64(ts, s, 0);
> +}


> +static long lan8841_ptp_do_aux_work(struct ptp_clock_info *ptp)
> +{
> +	struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> +							ptp_clock_info);
> +	struct skb_shared_hwtstamps *shhwtstamps;
> +	struct timespec64 ts;
> +	struct sk_buff *skb;
> +	u32 ts_header;
> +
> +	while ((skb = skb_dequeue(&ptp_priv->rx_queue)) != NULL) {
> +		lan8841_ptp_getseconds(ptp, &ts);

No need to call this once per frame.  It would be sufficent to call it
once every 2 seconds and cache the result.

> +		ts_header = __be32_to_cpu(LAN8841_SKB_CB(skb)->header->reserved2);
> +
> +		shhwtstamps = skb_hwtstamps(skb);
> +		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> +
> +		/* Check for any wrap arounds for the second part */
> +		if ((ts.tv_sec & GENMASK(1, 0)) < ts_header >> 30)
> +			ts.tv_sec -= GENMASK(1, 0) + 1;
> +
> +		shhwtstamps->hwtstamp =
> +			ktime_set((ts.tv_sec & ~(GENMASK(1, 0))) | ts_header >> 30,
> +				  ts_header & GENMASK(29, 0));
> +		LAN8841_SKB_CB(skb)->header->reserved2 = 0;
> +
> +		netif_rx(skb);
> +	}
> +
> +	return -1;
> +}

Thanks,
Richard