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

[Paolo Abeni <pabeni@redhat.com>]

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.

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

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

Cheers,

Paolo