RE: [PATCH net-next v4 02/13] net: renesas: rswitch: use device instead of net_device

[Michael Dege]

Hello Jakub,

Thank you for your review.

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Wednesday, May 13, 2026 3:36 AM
> To: Michael Dege <michael.dege@xxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; niklas.soderlund
> <niklas.soderlund@xxxxxxxxxxxx>; paul@xxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-renesas-
> soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v4 02/13] net: renesas: rswitch: use device instead of net_device
> 
> This is an AI-generated review of your patch. The human sending this email has considered the AI
> review valid, or at least plausible.
> ---
> net: renesas: rswitch: use device instead of net_device
> 
> This commit updates the driver to use struct device instead of struct net_device for queue allocation
> and freeing. This prepares the code for upcoming VLAN support where the net_device might not always be
> available.
> 
> > diff --git a/drivers/net/ethernet/renesas/rswitch_main.c
> > b/drivers/net/ethernet/renesas/rswitch_main.c
> > --- a/drivers/net/ethernet/renesas/rswitch_main.c
> > +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> > @@ -304,13 +304,13 @@ static int rswitch_gwca_queue_alloc_rx_buf(struct rswitch_gwca_queue *gq,
> >  	return -ENOMEM;
> >  }
> >
> > -static void rswitch_gwca_queue_free(struct net_device *ndev,
> > +static void rswitch_gwca_queue_free(struct device *dev,
> >  				    struct rswitch_gwca_queue *gq)  {
> >  	unsigned int i;
> >
> >  	if (!gq->dir_tx) {
> > -		dma_free_coherent(ndev->dev.parent,
> > +		dma_free_coherent(dev,
> >  				  sizeof(struct rswitch_ext_ts_desc) *
> >  				  (gq->ring_size + 1), gq->rx_ring, gq->ring_dma);
> 
> This wasn't introduced by this patch, but if rswitch_gwca_queue_alloc() fails during or before the
> dma_alloc_coherent() step, gq->rx_ring will remain NULL.
> 
> Since dma_free_coherent() is not guaranteed to handle NULL virtual addresses safely across all
> architectures, could this pass a NULL pointer and cause a crash? Does it make sense to conditionalize
> the frees here and in the tx path?
> 
> >  		gq->rx_ring = NULL;
> 
> I notice another pre-existing issue in the unquoted context just below this. When
> rswitch_gwca_queue_alloc() fails, gq->rx_bufs can contain NULL pointers.
> 
> Since the loop calls skb_free_frag(gq->rx_bufs[i]), and skb_free_frag() wraps page_frag_free() which
> does not check for NULL, would this result in an out-of-bounds page struct dereference and a kernel
> panic?
> 
> Would it be safer to add a NULL check before calling skb_free_frag()?
> 
> [ ... ]
> 
> > @@ -508,7 +506,7 @@ static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
> >  			index = (i + start_index) % gq->ring_size;
> >  			desc = &gq->rx_ring[index];
> >  			dma_addr = rswitch_desc_get_dptr(&desc->desc);
> > -			dma_unmap_single(ndev->dev.parent, dma_addr,
> > +			dma_unmap_single(dev, dma_addr,
> >  					 RSWITCH_MAP_BUF_SIZE, DMA_FROM_DEVICE);
> >  		}
> >  	}
> 
> I noticed a concurrency issue in this pre-existing error path. Earlier in this function, the driver
> transfers ownership of each descriptor to the hardware by setting desc->desc.die_dt and executing a
> dma_wmb().
> 
> If a subsequent dma_map_single() fails, this error path unmaps the previously mapped buffers but does
> not revoke hardware ownership by clearing desc->desc.die_dt.
> 
> Because the hardware was already granted ownership, could it actively DMA incoming packets into the
> unmapped memory before the device is finally halted? Would it be safer to clear the
?
> --
> pw-bot: cr

I reworked the whole function. Now this should not be the case anymore. I also added clearing of the 
ownership flag and issue a memory barrier before unmapping the buffers.

Best regards,

Michael