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

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 10 Jun 2024 15:59:35 +0200 Niklas Söderlund wrote:
> +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 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);
> +
> +		/* Update statistics. */
> +		ndev->stats.rx_packets++;
> +		ndev->stats.rx_bytes += pkt_len;
> +
> +		/* Update counters. */
> +		priv->cur_rx++;
> +		rx_packets++;
> +
> +		/* Stop processing descriptors if budget is consumed. */
> +		if (rx_packets >= budget)
> +			break;

budget can also be 0 when netpoll calls us to only process tx, don't
process even a single packet in that case. (BTW also double check you
never printk() under the spin lock you take in NAPI)

> +	}
> +
> +	/* 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 = netdev_alloc_skb(ndev,
> +					       PKT_BUF_SZ + RTSN_ALIGN - 1);

napi_alloc_skb()

> +			if (!skb)
> +				break;
> +			skb_reserve(skb, NET_IP_ALIGN);
> +			dma_addr = dma_map_single(ndev->dev.parent, skb->data,
> +						  le16_to_cpu(desc->info_ds),
> +						  DMA_FROM_DEVICE);
> +			if (dma_mapping_error(ndev->dev.parent, dma_addr))
> +				desc->info_ds = cpu_to_le16(0);
> +			desc->dptr = cpu_to_le32(dma_addr);
> +			skb_checksum_none_assert(skb);
> +			priv->rx_skb[entry] = skb;
> +		}
> +
> +		dma_wmb();
> +		desc->die_dt = DT_FEMPTY | D_DIE;
> +	}
> +
> +	priv->rx_ring[priv->num_rx_ring].die_dt = DT_LINK;
> +
> +	return rx_packets;
> +}

> +static struct net_device_stats *rtsn_get_stats(struct net_device *ndev)
> +{
> +	return &ndev->stats;
> +}

Please implement your own 64 bit stats:

	struct net_device_stats	stats; /* not used by modern drivers */

> +static int rtsn_do_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)
> +{
> +	if (!netif_running(ndev))
> +		return -ENODEV;
> +
> +	switch (cmd) {
> +	case SIOCGHWTSTAMP:
> +		return rtsn_hwstamp_get(ndev, ifr);
> +	case SIOCSHWTSTAMP:
> +		return rtsn_hwstamp_set(ndev, ifr);

please implement ndo_hwtstamp_{g,s}et instead

> +	default:
> +		break;
> +	}
> +
> +	return phy_do_ioctl_running(ndev, ifr, cmd);
> +}

> +static const struct of_device_id rtsn_match_table[] = {
> +	{.compatible = "renesas,r8a779g0-ethertsn", },

space missing

> +	{ /* Sentinel */ }
> +};

> +	priv->reset = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->reset)) {
> +		ret = -PTR_ERR(priv->reset);

PTR_ERR() should give you negative errno

> +	priv->ptp_priv->addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->ptp_priv->addr)) {
> +		ret = -PTR_ERR(priv->ptp_priv->addr);

ditto

> +	ret = rtsn_get_phy_params(priv);
> +	if (ret)
> +		goto error_alloc;

error_free (please name the labels after target)

> +	netdev_info(ndev, "MAC address %pM\n", ndev->dev_addr);

That's fairly unusual, why print the MAC address to logs?
-- 
pw-bot: cr