Received-SPF: pass (google.com: domain of lorenzo@kernel.org designates 172.105.4.254 as permitted sender) client-ip=172.105.4.254;
Date: Wed, 13 May 2026 11:43:51 +0200
From: Lorenzo Bianconi <lorenzo@kernel.org>
To: sashiko@lists.linux.dev
Cc: conor+dt@kernel.org, devicetree@vger.kernel.org, robh@kernel.org,
	krzk+dt@kernel.org
Subject: Re: [PATCH net-next v6 09/12] net: airoha: Do not stop GDM port if
 it is shared
Message-ID: <agRH12v6OeetUmtJ@lore-desk>
References: <20260511-airoha-eth-multi-serdes-v6-9-c899462c4f75@kernel.org>
 <20260512213116.7414FC2BCB0@smtp.kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="fA35iJO0kV4N5A4K"
Content-Disposition: inline
In-Reply-To: <20260512213116.7414FC2BCB0@smtp.kernel.org>


--fA35iJO0kV4N5A4K
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

> Thank you for your contribution! Sashiko AI review found 3 potential issu=
e(s) to consider:
> - [High] Stopping a net_device leaves its packets in the shared QDMA hard=
ware TX ring, causing a BUG_ON kernel panic or Use-After-Free when the hard=
ware later transmits them.
> - [Medium] When decreasing the MTU on a shared GDM port with multiple act=
ive users, the hardware MTU is not reduced to match the new global maximum.
> - [Low] The function parameter `netdev` is overwritten in a loop within `=
airoha_dev_stop()`.
> --
>=20
> commit 30071ffd90f181adfa0fb1ab6717d5d5cfa7e0e8
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
>=20
> net: airoha: Do not stop GDM port if it is shared
>=20
> Theoretically, in the current codebase, two independent net_devices can
> be connected to the same GDM port so we need to check the GDM port is not
> used by any other running net_device before setting the forward
> configuration to FE_PSE_PORT_DROP.
>=20
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/eth=
ernet/airoha/airoha_eth.c
> > index 4efd36779453e..aed182c7a5e17 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > @@ -1759,6 +1769,7 @@ static int airoha_dev_stop(struct net_device *net=
dev)
> >  	struct airoha_gdm_dev *dev =3D netdev_priv(netdev);
> >  	struct airoha_gdm_port *port =3D dev->port;
> >  	struct airoha_qdma *qdma =3D dev->qdma;
> > +	u32 len =3D 0;
> >  	int i;
> > =20
> >  	netif_tx_disable(netdev);
> > @@ -1766,8 +1777,23 @@ static int airoha_dev_stop(struct net_device *ne=
tdev)
> >  	for (i =3D 0; i < netdev->num_tx_queues; i++)
> >  		netdev_tx_reset_subqueue(netdev, i);
>=20
> If another device is still using the QDMA (qdma->users > 1), the hardware=
 TX
> ring is not cleaned up. NAPI polling is bound to a dummy device and conti=
nues
> running.
>=20
> When NAPI later processes the stopped device's pending TX completions, it
> unconditionally calls netdev_tx_completed_queue() which invokes
> dql_completed().
>=20
> Since BQL was reset here, will this trigger a BUG_ON in
> lib/dynamic_queue_limits.c? Furthermore, if the device was unregistered,
> would the skb->dev access in the completion handler result in a
> use-after-free?

This issue is not introduced in this patch.

>=20
> > =20
> > -	airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id),
> > -				    FE_PSE_PORT_DROP);
> > +	for (i =3D 0; i < ARRAY_SIZE(port->devs); i++) {
> > +		if (!port->devs[i])
> > +			continue;
> > +
> > +		netdev =3D port->devs[i]->dev;
>=20
> This isn't a bug, but this loop reassigns the local parameter netdev. Whi=
le
> it might be functionally harmless right now because netdev isn't derefere=
nced
> later in this function, could this create issues for future modifications
> expecting netdev to still point to the device being stopped?

ack, I will fix it in v7.

>=20
> > +		if (netif_running(netdev))
> > +			len =3D max_t(u32, len, netdev->mtu);
> > +	}
> > +	len +=3D ETH_HLEN + ETH_FCS_LEN;
> > +	airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> > +		      GDM_LONG_LEN_MASK,
> > +		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > +
> [ ... ]
> > @@ -1919,13 +1945,18 @@ static void airoha_dev_get_stats64(struct net_d=
evice *netdev,
> >  static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
> >  {
> >  	struct airoha_gdm_dev *dev =3D netdev_priv(netdev);
> > +	u32 cur_len, len =3D ETH_HLEN + mtu + ETH_FCS_LEN;
> >  	struct airoha_gdm_port *port =3D dev->port;
> > -	u32 len =3D ETH_HLEN + mtu + ETH_FCS_LEN;
> >  	struct airoha_eth *eth =3D dev->eth;
> > =20
> > -	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> > -		      GDM_LONG_LEN_MASK,
> > -		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > +	cur_len =3D airoha_fe_get(eth, REG_GDM_LEN_CFG(port->id),
> > +				GDM_LONG_LEN_MASK);
> > +	if (len > cur_len || !port->users ||
> > +	    (port->users =3D=3D 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));
>=20
> If there are multiple running devices on the shared port and they all low=
er
> their MTU, this condition prevents the hardware MTU from being updated to
> match the new global maximum. Should this iterate over port->devs to comp=
ute
> the actual maximum MTU of all running devices, similar to how it is handl=
ed
> in airoha_dev_stop()?

According to my understanding, airoha_dev_change_mtu() is running with RTNL
lock held so we will always have the max configured mtu in cur_len.

Regards,
Lorenzo

>=20
> > +
> >  	WRITE_ONCE(netdev->mtu, mtu);
> > =20
> >  	return 0;
>=20
> --=20
> Sashiko AI review =B7 https://sashiko.dev/#/patchset/20260511-airoha-eth-=
multi-serdes-v6-0-c899462c4f75@kernel.org?part=3D9

--fA35iJO0kV4N5A4K
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCagRH1wAKCRA6cBh0uS2t
rM31AQDunwBJ0DVCS5kAHpHwul4CaN/W8nGCUKRkGWyRHdTNNAD9HDklGrcRS6YR
qT2san+wDl2cbYm9Qr6rnVWH8iYZ8QM=
=oOYX
-----END PGP SIGNATURE-----

--fA35iJO0kV4N5A4K--