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

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2024-06-20 at 13:50 +0200, Niklas Söderlund wrote:
> 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?

I understand your pain with such change. Hopefully this will not need
many more revisions, so I guess you can keep a single patch.

WRT to the previous advice, I guess the controversial word is 'basic':
this patch is ~2k LoC, considerably more then a comfortable (to me)
size.

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

In the context of a largish refactor, then I suggest additional
investigating replacing napi_gro_receive() with napi_gro_frags().

The latter should provide the best performances for GRO-ed traffic.

Cheers,

Paolo