Re: [net-next,v22,4/7] net: mtip: Add net_device_ops functions to the L2 switch driver

Re: [net-next,v22,4/7] net: mtip: Add net_device_ops functions to the L2 switch driver

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mtip: Add net_device_ops functions to the L2 switch driver

This patch adds net_device_ops callbacks for the MTIP L2 switch driver,
including transmit, multicast configuration, MAC address handling, and
timeout recovery functions.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c

[ ... ]

> +static netdev_tx_t mtip_start_xmit_port(struct sk_buff *skb,
> +					struct net_device *dev, int port)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	unsigned short status;
> +	unsigned int index;
> +	struct cbd_t *bdp;
> +	void *bufaddr;
> +
> +	spin_lock_bh(&fep->hw_lock);

[ ... ]

> +	/* Save skb pointer. */
> +	fep->tx_skbuff[index] = skb;

mtip_start_xmit_port() stores sk_buffs in fep->tx_skbuff[index] and creates
DMA mappings, but mtip_switch_tx() which should complete TX operations is
empty:

static void mtip_switch_tx(struct switch_enet_private *fep)
{
}

The NAPI poll handler mtip_rx_napi() calls mtip_switch_tx() to reclaim
transmitted buffers. Since it does nothing, won't every transmitted packet
leak the skb and DMA mapping? Also, dirty_tx is never advanced, so the TX
ring will fill (cur_tx == dirty_tx) and stop accepting packets.

jakub: maybe add a TODO comment transiently here, since later patch implements
       the cleanup?

> +static void mtip_timeout(struct net_device *dev, unsigned int txqueue)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +
> +	dev->stats.tx_errors++;

Jakub Kicinski noted in v16 review that tx_errors are already counted by
the network stack, and the statistic is exposed per-queue in sysfs. Does
incrementing tx_errors here double-count timeout events?

https://lore.kernel.org/all/20250725151618.0bc84bdb@kernel.org/

> +	DO_ONCE(mtip_print_hw_state, dev);
> +
> +	schedule_work(&priv->tx_timeout_work);
> +}

Re: [net-next,v22,4/7] net: mtip: Add net_device_ops functions to the L2 switch driver

[Łukasz Majewski]

Hi Jakub,

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see
> https://netdev-ai.bots.linux.dev/ai-local.html ---
> net: mtip: Add net_device_ops functions to the L2 switch driver
> 
> This patch adds net_device_ops callbacks for the MTIP L2 switch
> driver, including transmit, multicast configuration, MAC address
> handling, and timeout recovery functions.
> 
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c ---
> > a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c +++
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c  
> 
> [ ... ]
> 
> > +static netdev_tx_t mtip_start_xmit_port(struct sk_buff *skb,
> > +					struct net_device *dev,
> > int port) +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	unsigned short status;
> > +	unsigned int index;
> > +	struct cbd_t *bdp;
> > +	void *bufaddr;
> > +
> > +	spin_lock_bh(&fep->hw_lock);  
> 
> [ ... ]
> 
> > +	/* Save skb pointer. */
> > +	fep->tx_skbuff[index] = skb;  
> 
> mtip_start_xmit_port() stores sk_buffs in fep->tx_skbuff[index] and
> creates DMA mappings, but mtip_switch_tx() which should complete TX
> operations is empty:
> 
> static void mtip_switch_tx(struct switch_enet_private *fep)
> {
> }
> 
> The NAPI poll handler mtip_rx_napi() calls mtip_switch_tx() to reclaim
> transmitted buffers. Since it does nothing, won't every transmitted
> packet leak the skb and DMA mapping? Also, dirty_tx is never
> advanced, so the TX ring will fill (cur_tx == dirty_tx) and stop
> accepting packets.
> 
> jakub: maybe add a TODO comment transiently here, since later patch
> implements the cleanup?

The mtip_switch_tx() is empty as we have agreed, that I will divide
this driver to several patches to ease the review.

Adding TODO seems to only make AI review happy, as:

- The patch which adds support for FEC_MTIP_L2SW in Kconfig will be
  sent after the driver is accepted to net-next

- Those commits are even now bisectable when FEC_MTIP_L2SW is
  enabled (when I test the setup).

Anyway, if you still would like to have the TODO comment, then please
give me a hint how it shall be written to make the AI happy...



And maybe a few my thoughts:

1. AI review seems to bring each time different issues - even the
"grammatic" ones were not provided with the first AI generated review.

2. I have tried to setup claudie> to run the patch set through it -
however, it requires a paid account on a cloud/AI vendor (and I guess
that different vendors' AI engines produce different output for the same
"AI prompt")?


Anyway, I do appreciate the AI review - it provides very deep insights
through the code.

> 
> > +static void mtip_timeout(struct net_device *dev, unsigned int
> > txqueue) +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +
> > +	dev->stats.tx_errors++;  
> 
> Jakub Kicinski noted in v16 review that tx_errors are already counted
> by the network stack, and the statistic is exposed per-queue in
> sysfs. Does incrementing tx_errors here double-count timeout events?
> 
> https://lore.kernel.org/all/20250725151618.0bc84bdb@kernel.org/
> 

Yes, this shall been removed.

> > +	DO_ONCE(mtip_print_hw_state, dev);
> > +
> > +	schedule_work(&priv->tx_timeout_work);
> > +}  



-- 
Best regards,

Łukasz Majewski

Re: [net-next,v22,4/7] net: mtip: Add net_device_ops functions to the L2 switch driver

[Andrew Lunn]

> > jakub: maybe add a TODO comment transiently here, since later patch
> > implements the cleanup?
> 
> The mtip_switch_tx() is empty as we have agreed, that I will divide
> this driver to several patches to ease the review.
> 
> Adding TODO seems to only make AI review happy, as:
> 
> - The patch which adds support for FEC_MTIP_L2SW in Kconfig will be
>   sent after the driver is accepted to net-next
> 
> - Those commits are even now bisectable when FEC_MTIP_L2SW is
>   enabled (when I test the setup).
> 
> Anyway, if you still would like to have the TODO comment, then please
> give me a hint how it shall be written to make the AI happy...

I would not make too much effort in keeping the AI happy, for
something we understand is transient. It is currently not a gate for
acceptance.

> And maybe a few my thoughts:
> 
> 1. AI review seems to bring each time different issues - even the
> "grammatic" ones were not provided with the first AI generated review.

The rules are being tweaked as we gain experience with it, so i would
not expect it to produce the same output every time. Even if it was
stable, it is not clear to me if it is reproducible.
 
> 2. I have tried to setup claudie> to run the patch set through it -
> however, it requires a paid account on a cloud/AI vendor (and I guess
> that different vendors' AI engines produce different output for the same
> "AI prompt")?

I expect so. It is clearly not checkpatch.pl and the like were you can
run it 100 times and get the same answer every time. But i think that
is understood. So long as there has been a discussion about its
output, patches should get merged even if there are still AI
comments. It is just another tool used in the review conversations.

	Andrew