Re: [net-next,v9] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hello Paolo,

Thanks for your feedback.

On 2024-06-20 13:13:21 +0200, Paolo Abeni wrote:
> On Tue, 2024-06-18 at 11:08 +0200, Niklas Söderlund wrote:
> > Add initial support for Renesas Ethernet-TSN End-station device of R-Car
> > V4H. The Ethernet End-station can connect to an Ethernet network using a
> > 10 Mbps, 100 Mbps, or 1 Gbps full-duplex link via MII/GMII/RMII/RGMII.
> > Depending on the connected PHY.
> > 
> > The driver supports Rx checksum and offload and hardware timestamps.
> > 
> > While full power management and suspend/resume is not yet supported the
> > driver enables runtime PM in order to enable the module clock. While
> > explicit clock management using clk_enable() would suffice for the
> > supported SoC, the module could be reused on SoCs where the module is
> > part of a power domain.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> I'm sorry for giving such fundamental feedback this late, but I think
> this should be split in series to simplify the review process.
> 
> You could e.g. introduce the defines and probe in patch 1, the rx path
> in patch 2, and the tx path in patch 3.

I have been given the opposite advice in the past, to add a basic driver 
in one single commit. All be it this was in other subsystems. This 
already have been thru a lot of review, do you feel strongly about this 
or can I note it down for how netdev prefers work do be done in future?

> 
> > +static int rtsn_rx(struct net_device *ndev, int budget)
> > +{
> > +	struct rtsn_private *priv = netdev_priv(ndev);
> > +	unsigned int ndescriptors;
> > +	unsigned int rx_packets;
> > +	unsigned int i;
> > +	bool get_ts;
> > +
> > +	get_ts = priv->ptp_priv->tstamp_rx_ctrl &
> > +		RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
> > +
> > +	ndescriptors = priv->dirty_rx + priv->num_rx_ring - priv->cur_rx;
> > +	rx_packets = 0;
> > +	for (i = 0; i < ndescriptors; i++) {
> > +		const unsigned int entry = priv->cur_rx % priv->num_rx_ring;
> > +		struct rtsn_ext_ts_desc *desc = &priv->rx_ring[entry];
> > +		struct sk_buff *skb;
> > +		dma_addr_t dma_addr;
> > +		u16 pkt_len;
> > +
> > +		/* Stop processing descriptors if budget is consumed. */
> > +		if (rx_packets >= budget)
> > +			break;
> > +
> > +		/* Stop processing descriptors on first empty. */
> > +		if ((desc->die_dt & DT_MASK) == DT_FEMPTY)
> > +			break;
> > +
> > +		dma_rmb();
> > +		pkt_len = le16_to_cpu(desc->info_ds) & RX_DS;
> > +
> > +		skb = priv->rx_skb[entry];
> > +		priv->rx_skb[entry] = NULL;
> > +		dma_addr = le32_to_cpu(desc->dptr);
> > +		dma_unmap_single(ndev->dev.parent, dma_addr, PKT_BUF_SZ,
> > +				 DMA_FROM_DEVICE);
> > +
> > +		/* Get timestamp if enabled. */
> > +		if (get_ts) {
> > +			struct skb_shared_hwtstamps *shhwtstamps;
> > +			struct timespec64 ts;
> > +
> > +			shhwtstamps = skb_hwtstamps(skb);
> > +			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > +
> > +			ts.tv_sec = (u64)le32_to_cpu(desc->ts_sec);
> > +			ts.tv_nsec = le32_to_cpu(desc->ts_nsec & cpu_to_le32(0x3fffffff));
> > +
> > +			shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
> > +		}
> > +
> > +		skb_put(skb, pkt_len);
> > +		skb->protocol = eth_type_trans(skb, ndev);
> > +		netif_receive_skb(skb);
> 
> Since the driver uses napi this should be napi_gro_receive().

Thanks, will fix.

> 
> > +
> > +		/* Update statistics. */
> > +		priv->stats.rx_packets++;
> > +		priv->stats.rx_bytes += pkt_len;
> > +
> > +		/* Update counters. */
> > +		priv->cur_rx++;
> > +		rx_packets++;
> > +	}
> > +
> > +	/* Refill the RX ring buffers */
> > +	for (; priv->cur_rx - priv->dirty_rx > 0; priv->dirty_rx++) {
> > +		const unsigned int entry = priv->dirty_rx % priv->num_rx_ring;
> > +		struct rtsn_ext_ts_desc *desc = &priv->rx_ring[entry];
> > +		struct sk_buff *skb;
> > +		dma_addr_t dma_addr;
> > +
> > +		desc->info_ds = cpu_to_le16(PKT_BUF_SZ);
> > +
> > +		if (!priv->rx_skb[entry]) {
> > +			skb = napi_alloc_skb(&priv->napi,
> > +					     PKT_BUF_SZ + RTSN_ALIGN - 1);
> 
> skb allocation is preferred at receive time, so that the sk_buff itself
> is hot in the cache. Adapting to such style would likely require a
> larger refactor, so feel free to avoid it.

This is good feedback. There are advanced features in TSN that I would 
like to work on in the future. One of them is to improve the Rx path to 
support split descriptors allowing for larger MTU. That too would 
require invasive changes in this code. I will make a note of it and try 
to do both.

-- 
Kind Regards,
Niklas Söderlund