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

["Łukasz Majewski" <lukasz.majewski@mailbox.org>]

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 the net_device_ops callback implementations for the
> MTIP L2 switch driver including transmit, timeout handling, multicast
> setup, MAC address configuration, and port parent ID retrieval.
> 
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c index
> > 9c21a3d29c1c..a6424ee1a9d9 100644 ---
> > 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) +{  
> 
> [ ... ]
> 
> > +	/* Push the data cache so the CPM does not get stale memory
> > +	 * data.
> > +	 */
> > +	bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev, bufaddr,
> > +					  MTIP_SWITCH_TX_FRSIZE,
> > +					  DMA_TO_DEVICE);  
> 
> The dma_map_single() call here creates a DMA mapping, but
> mtip_switch_tx() at this point in the series is an empty stub that
> never calls dma_unmap_single().

Yes.

> Is the intention for this to be
> resolved by a later patch in the series? Looking ahead, a4c3fa1c66c9
> ("net: mtip: Add mtip_switch_{rx|tx} functions to the L2 switch
> driver") does add the full TX completion handler with the
> dma_unmap_single() call. However, this commit on its own could cause
> DMA resource exhaustion if used standalone (e.g., during bisection).
> 

The MTIP Kconfig define would not be available at this point of the
series.

The split is done as part of the agreement of how that large driver
shall be added.

The mtip_switch_rx|tx is a stub at this point to ease the review.

> Would it make sense to merge these two patches together, or at minimum
> have a note in the commit message explaining the dependency?
> 

@Jakub - If possible I would keep things as they are now.

> [ ... ]



-- 
Best regards,

Łukasz Majewski