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@xxxxxxxxxxxx>
> >
> > 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
