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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

On May 17, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Flawed hardware MTU recalculation logic in airoha_dev_change_mtu() for shared GDM ports
> --
> 
> commit 28e02ac4611575e19c4f6eef3fa74c11d4debb6e
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Do not stop GDM port if it is shared
> 
> This commit updates the driver to allow multiple devices to share the same
> GDM port. It stops the port only when no other devices are using it and
> adjusts the hardware MTU to accommodate all active devices.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 8558790eca644..c70e3ace38e25 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> 
> [ ... ]
> 
> > @@ -1917,13 +1945,18 @@ static void airoha_dev_get_stats64(struct net_device *netdev,
> >  static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
> >  {
> >  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > +	u32 cur_len, len = ETH_HLEN + mtu + ETH_FCS_LEN;
> >  	struct airoha_gdm_port *port = dev->port;
> > -	u32 len = ETH_HLEN + mtu + ETH_FCS_LEN;
> >  	struct airoha_eth *eth = dev->eth;
> >  
> > -	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> > -		      GDM_LONG_LEN_MASK,
> > -		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > +	cur_len = airoha_fe_get(eth, REG_GDM_LEN_CFG(port->id),
> > +				GDM_LONG_LEN_MASK);
> > +	if (len > cur_len || !port->users ||
> > +	    (port->users == 1 && netif_running(netdev)))
> > +		airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> > +			      GDM_LONG_LEN_MASK,
> > +			      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > +
> 
> The commit message states that the driver must "always set in GDM_LONG_LEN_MASK
> field of REG_GDM_LEN_CFG register the maximum MTU of all running net_devices
> connected to the same GDM port."
> 
> Does this logic successfully recalculate the MTU when the MTU of a running
> device is decreased while sibling devices are also running? In that scenario,
> len > cur_len evaluates to false, which appears to skip the update and leave
> the hardware MTU stuck at the previous larger value.
> 
> Additionally, if an administratively down device has its MTU increased,
> len > cur_len can evaluate to true. Does this prematurely increase the
> hardware MTU, exposing the running sibling devices to larger frames based
> on a stopped device's configuration?
> 
> Would it be more accurate to iterate over the devices to compute the
> actual maximum MTU of all active devices here, similar to the approach taken
> in airoha_dev_stop()?

ack, I will fix it in v8.

Regards,
Lorenzo

> 
> >  	WRITE_ONCE(netdev->mtu, mtu);
> >  
> >  	return 0;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260516-airoha-eth-multi-serdes-v7-0-99e0093303e2@kernel.org?part=8

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