RE: [PATCH net-next v4 05/13] net: renesas: rswitch: add exception path for packets with unknown dst MAC

[Michael Dege]

Hello Jakub,

Thank you for your comments.

> -----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 05/13] net: renesas: rswitch: add exception path for packets with
> unknown dst MAC
> 
> 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: add exception path for packets with unknown dst MAC
> 
> Creates a queue for the exception path to handle packets with unknown MAC addresses. These packets are
> sent to the CPU for MAC learning and broadcast.
> 
> > diff --git a/drivers/net/ethernet/renesas/rswitch_main.c
> > b/drivers/net/ethernet/renesas/rswitch_main.c
> > index e05c42db5f4c8..bf26c1a3384aa 100644
> > --- a/drivers/net/ethernet/renesas/rswitch_main.c
> > +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> > @@ -682,6 +682,34 @@ static int rswitch_rxdmac_init(struct
> > rswitch_private *priv, unsigned int index)
> [ ... ]
> > +static int rswitch_shared_rx_queue_alloc(struct rswitch_private
> > +*priv) {
> [ ... ]
> > +	err = rswitch_gwca_queue_alloc(NULL, priv, gwca->l2_shared_rx_queue, false, RX_RING_SIZE);
> > +	if (err < 0) {
> > +		rswitch_gwca_put(priv, gwca->l2_shared_rx_queue);
> > +		return err;
> > +	}
> > +
> > +	return rswitch_gwca_queue_ext_ts_format(dev, priv,
> > +gwca->l2_shared_rx_queue); }
> 
> If rswitch_gwca_queue_ext_ts_format() fails, does this leak the DMA resources allocated by
> rswitch_gwca_queue_alloc() and the queue bitmask index acquired via rswitch_gwca_get()?

I added a call to rswitch_gwca_queue_free() for cases when rswitch_gwca_queue_alloc() fails.
This cleans up the DMA resources and the bitmask.
> 
> [ ... ]
> > @@ -954,15 +993,40 @@ static int rswitch_poll(struct napi_struct
> > *napi, int budget)
> [ ... ]
> > -static void rswitch_queue_interrupt(struct net_device *ndev)
> > +static void rswitch_queue_interrupt(struct rswitch_private *priv,
> > +struct rswitch_gwca_queue *gq)
> >  {
> > -	struct rswitch_device *rdev = netdev_priv(ndev);
> > +	struct rswitch_ext_ts_desc *desc;
> > +	struct rswitch_device *rdev;
> > +	struct net_device *ndev;
> > +	unsigned int rx_q_index;
> > +	u32 spn;
> > +
> > +	rdev = netdev_priv(gq->ndev);
> 
> Since gq->ndev is initialized to NULL for the shared exception queue, does this unconditionally
> dereference a NULL pointer?
> 
> > +	rx_q_index = rdev->rx_queue->index;
> 
> And compute an invalid memory address here?

I reworked this to make this more robust. This should not happen any more.
> 
> > +
> > +/* If we receive a shared queue through the exception path, it will
> > +be missing the ndev
> > + * pointer. This needs to be added to be able to determine from which
> > +port the packet was
> > + * received. Then we temporarily exchange the rx_queue pointer in
> > +rdev. This will be
> > + * restored after the packet has been processed.
> > + */
> > +
> > +	if (gq->index == priv->gwca.l2_shared_rx_queue->index) {
> > +		desc = &gq->rx_ring[gq->cur];
> > +		spn = FIELD_GET(SPN, desc->info1);
> 
> Because desc->info1 is a little-endian 64-bit field (__le64), will applying a host-endian bitfield
> mask without converting it via le64_to_cpu() first yield garbage values on big-endian architectures?
> 
> > +		ndev = priv->rdev[spn]->ndev;
> 

I added the conversion le64_to_cpu(). This also removed the error from the sparse compilation.

> Can a malformed or byte-swapped descriptor result in an out-of-bounds memory read (spn >=
> RSWITCH_NUM_PORTS) here?
> 
> > +		rdev = netdev_priv(ndev);
> > +		gq->ndev = ndev;
> > +		/* store original rx_queue */
> > +		rdev->rx_old_queue = rdev->rx_queue;
> > +		rdev->rx_queue = gq;
> 
> Does this lockless swap of queue pointers in hardirq context create a data race with rswitch_poll(),
> which reads and restores them in NAPI softirq context?
> 
> Furthermore, if napi_schedule_prep() returns false because NAPI is already running, the interrupt is
> not disabled. If a subsequent interrupt for the same port executes this swap again, will it overwrite
> rdev->rx_old_queue with the shared queue pointer (gq) and permanently lose the normal RX queue?
> 
> Also, l2_shared_rx_queue aggregates packets from all physical ports. Since this reads the source port
> (spn) of only the first pending packet and schedules NAPI for that specific network interface, won't
> the NAPI polling loop consume subsequent packets in the batch and incorrectly attribute them to the
> first packet's interface?

Since this is the slow path, I don't expect too many packets using this path. Especially since the 
environment inside a vehicle is very homogeneous I would not expect this to happen.

The vast majority of packets will not be seen by the CPU, because they will be handled in HW.

> 
> > +	}
> >
> >  	if (napi_schedule_prep(&rdev->napi)) {
> [ ... ]
> > @@ -1646,6 +1721,7 @@ static int rswitch_stop(struct net_device *ndev)
> [ ... ]
> >  	spin_lock_irqsave(&rdev->priv->lock, flags);
> >  	rswitch_enadis_data_irq(rdev->priv, rdev->tx_queue->index, false);
> >  	rswitch_enadis_data_irq(rdev->priv, rdev->rx_queue->index, false);
> > +	rswitch_enadis_data_irq(rdev->priv,
> > +rdev->priv->gwca.l2_shared_rx_queue->index, false);
> 
> Since l2_shared_rx_queue is a global resource used for MAC learning exception packets across all
> ports, does stopping a single network interface break the globally shared exception path for all other
> active interfaces?

No, this has been tested and this has not been observed.

Best regards,

Michael