Re: [PATCH net-next v2 10/11] net: airoha: Do not stop GDM port if it is shared

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3201 bytes --]

> 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: airoha: Do not stop GDM port if it is shared
> 
> This patch introduces a port->users reference counter to prevent setting
> the GDM port forward configuration to FE_PSE_PORT_DROP if another net_device
> is still using the shared GDM port.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 0780fb8aacfb..3f9bce8c5370 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > @@ -1681,9 +1682,6 @@ static int airoha_dev_stop(struct net_device *netdev)
> >  	for (i = 0; i < ARRAY_SIZE(qdma->q_tx); i++)
> >  		netdev_tx_reset_subqueue(netdev, i);
> >  
> > -	airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id),
> > -				    FE_PSE_PORT_DROP);
> > -
> >  	if (atomic_dec_and_test(&qdma->users)) {
> >  		airoha_qdma_clear(qdma, REG_QDMA_GLOBAL_CFG,
> >  				  GLOBAL_CFG_TX_DMA_EN_MASK |
> 
> While this patch aims to support shared GDM ports, does the remaining
> unconditional call to airoha_set_vip_for_gdm_port(dev, false) at the start
> of airoha_dev_stop() break other active devices sharing the port?
> 
> static int airoha_dev_stop(struct net_device *netdev)
> {
>         ...
>         err = airoha_set_vip_for_gdm_port(dev, false);
>         ...
> }
> 
> This appears to clear the REG_FE_VIP_PORT_EN and REG_FE_IFC_PORT_EN hardware
> bits for the physical port, which might instantly kill traffic for any other
> active interface using the shared port.
> 
> Similarly, in airoha_dev_open() and airoha_dev_change_mtu(), the
> REG_GDM_LEN_CFG (MTU) is overwritten unconditionally. If a device with a
> smaller MTU is brought up, will it overwrite the shared port's MTU limit
> and cause the hardware to silently drop larger packets for the other device?

ack, I will fix it in v3.

> 
> [ ... ]
> > @@ -1697,6 +1695,11 @@ static int airoha_dev_stop(struct net_device *netdev)
> >  		}
> >  	}
> >  
> > +	if (atomic_dec_and_test(&port->users))
> > +		airoha_set_gdm_port_fwd_cfg(qdma->eth,
> > +					    REG_GDM_FWD_CFG(port->id),
> > +					    FE_PSE_PORT_DROP);
> > +
> >  	return 0;
> >  }
> 
> Does moving this FE_PSE_PORT_DROP configuration to the end of airoha_dev_stop()
> cause a race condition when stopping the device?
> 
> In airoha_dev_open(), the driver correctly enables the QDMA engine before
> configuring the GDM port to forward packets. A safe teardown typically
> mirrors this by first stopping the packet source (setting FE_PSE_PORT_DROP)
> and then disabling the destination DMA engine.
> 
> By moving airoha_set_gdm_port_fwd_cfg() after the airoha_qdma_clear() block,
> when both qdma->users and port->users reach zero, the QDMA engine and its
> queues are shut down while the GDM port is still actively forwarding packets.
> 
> Could incoming packets be forwarded to a disabled DMA engine during this
> window, risking hardware lockups or buffer leaks?

ack, I will fix it in v3.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]