Re: [PATCH net-next v5 09/12] net: airoha: Do not stop GDM port if it is shared

Re: [PATCH net-next v5 09/12] net: airoha: Do not stop GDM port if it is shared

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The `GDM_STAG_EN_MASK` configuration is port-global but is modified unconditionally per net_device, leading to silent clobbering when multiple devices share a port.
> - [Medium] The hardware MTU (`REG_GDM_LEN_CFG`) is not recalculated when a device sharing the port is stopped, leaving the MAC configured to accept inappropriately large frames.
> --
> 
> commit b896e320dcf0d56db7e0654191126c23e6f485fd
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> net: airoha: Do not stop GDM port if it is shared
> 
> This commit introduces a users counter to the GDM port structure to allow
> multiple independent net_device instances to share the same hardware port
> without prematurely stopping it. It also conditionally updates the MTU so
> that the largest MTU requested by any sharing device is used.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index be31f639f4be3..ebb24e60f1de3 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -1733,10 +1733,20 @@ static int airoha_dev_open(struct net_device *netdev)
> >  		airoha_fe_clear(qdma->eth, REG_GDM_INGRESS_CFG(port->id),
> >  				GDM_STAG_EN_MASK);
> 
> Since GDM_STAG_EN_MASK is a port-global configuration, will this clear the 
> flag for the entire port if a secondary non-DSA net_device is brought up?
> 
> If the primary master device relies on this hardware tag parsing, it appears
> a secondary device could silently break it by clearing the mask during
> its open routine. Should this configuration be protected or refcounted when
> multiple devices share a port?

This is not a real issue since the hw design supports DSA just on GDM1 where we
can't configure multiple net_devices.

> 
> > -	airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> > -		      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> > -		      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> > -		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > +	cur_len = airoha_fe_get(qdma->eth, REG_GDM_LEN_CFG(port->id),
> > +				GDM_LONG_LEN_MASK);
> > +	if (!port->users || len > cur_len) {
> > +		/* Opening a sibling net_device with a larger MTU updates the
> > +		 * MTU of already running devices. This is required to allow
> > +		 * multiple net_devices wht different MTUs to share the same
> > +		 * QDMA block.
> > +		 */
> > +		airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> > +			      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> > +			      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> > +			      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > +	}
> > +	port->users++;
> 
> [ ... ]
> 
> > @@ -1766,8 +1776,10 @@ static int airoha_dev_stop(struct net_device *netdev)
> >  	for (i = 0; i < netdev->num_tx_queues; 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 (!--port->users)
> > +		airoha_set_gdm_port_fwd_cfg(qdma->eth,
> > +					    REG_GDM_FWD_CFG(port->id),
> > +					    FE_PSE_PORT_DROP);
> 
> If multiple devices are sharing the port and the device with the largest MTU
> is stopped, the hardware MTU in REG_GDM_LEN_CFG doesn't appear to be
> recalculated or shrunk.
> 
> Since the MTU is only expanded in airoha_dev_open() and airoha_dev_change_mtu(),
> does this leave the MAC configured to accept inappropriately large frames
> for the remaining devices indefinitely?

ack, I will fix it in v6.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260509-airoha-eth-multi-serdes-v5-0-805e38edc2aa@kernel.org?part=9

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