From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1D20FCD3447 for ; Sat, 9 May 2026 09:42:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=++TpRzL3fTcDfWTuiUcGrOd3LT5zsko8y1dyuS7e1f4=; b=MWOPnclO+NnxKXraLHRu/FOT0b Zax6/cEfqTMtj6OMaFX+SHlyQ/sz032ye9jb8Ml6PuW64UcbrD+jGSBINS89ftHjeDdiz1ddcsfH9 OxTFUoBtpO2DE+nnAPY8YRyG68G1tY3s6KDsdlEhbSeIRWFnLIWCKYexYvw/ZUa8V3nZ9kL+ZYTLK WaNML/iR5BhsI0gAi+Os41784h6SycFDtTAwnu8rGqNk87M6+qLMN3fxACj0EPbl4gIcDXbYk5boT JiN1ehwr5LYW8cA6LRvc+wnjtCuWvVZ0nMZ3o4qdSMfNmwjB26dH3ZPrtPXtqVB6wdUA0OO3Z5Aku ABmITq7A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wLeCF-00000008gyG-2I0V; Sat, 09 May 2026 09:42:07 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wLeCC-00000008gxV-0pF1; Sat, 09 May 2026 09:42:05 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id 4F76F44217; Sat, 9 May 2026 09:42:03 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A9F23C2BCB2; Sat, 9 May 2026 09:42:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1778319723; bh=K7XW3fDrAi/SJUEYupM1iZBtmsSrwJ2WcBoDs0GiXxg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Lk+pKtRgFMongdzhfKR4yRp3XwmtqK900PBJqUpGpyvNcTkt8ZYsevmTxvRsL3O2z o7lW7WOaYHmWG1Ya+c/32+njK33gfkUEpVz5aRhqZ9DldJwXvKXzes+L6vSOLitcnh JvDMNOenzFLp6IhorBIwVUlEzOheS4ILqATLz2B/WEqmThCKq7aTskkgoV/Y4Hjejl wBtjkZdAWTT+jFwGvt9d86cfq39OINSSfEejj/uO9y9Ahrf6gVZr1X0rEncKSA1CkH yMTDRkk13xaMIgZvflOLW01hRock4FvFRM3wbN1NyNBl/ZUy3IzrEkyno5l3zGgaY3 2dB7F7C5Vq1Yg== Date: Sat, 9 May 2026 11:42:00 +0200 From: Lorenzo Bianconi To: Jakub Kicinski Cc: andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, ansuelsmth@gmail.com, benjamin.larsson@genexis.eu, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, netdev@vger.kernel.org, devicetree@vger.kernel.org, madhur.agrawal@airoha.com Subject: Re: [PATCH net-next v4 09/10] net: airoha: Introduce WAN device flag Message-ID: References: <20260507-airoha-eth-multi-serdes-v4-9-af613b61ae02@kernel.org> <20260509014615.3558164-1-kuba@kernel.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="cVtlXQ1B6cbL+hCZ" Content-Disposition: inline In-Reply-To: <20260509014615.3558164-1-kuba@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260509_024204_284435_0A270EFE X-CRM114-Status: GOOD ( 45.36 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org --cVtlXQ1B6cbL+hCZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable > 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: Introduce WAN device flag >=20 > This commit replaces the port->id =3D=3D 1 LAN/WAN test with a per-dev > PRIV_FLAG_WAN bit that is set during airoha_dev_init() based on the > port id and the presence of other WAN devices. It also extracts the > QDMA selection/PPE cpu port programming into a new helper and reworks > the GDM2 loopback path to key off the new flag. >=20 > A few questions about the new policy and error paths. >=20 > [Medium] > The commit message says: >=20 > Introduce WAN flag to specify if a given device is used to > transmit/receive WAN or LAN traffic. [...] Please note it is possible > to specify multiple LAN devices but just a single WAN one. >=20 > Under the old helper (port->id =3D=3D 1), GDM2/GDM3/GDM4 were all treated= as > WAN. Under the new logic, when GDM2 (eth->ports[1]) is present, the > GDM3/GDM4 case breaks out of the switch without setting PRIV_FLAG_WAN, so > GDM3 and GDM4 are now classified as LAN. That flips the MAC register > (REG_FE_WAN_MAC_H -> REG_FE_LAN_MAC_H in airoha_set_macaddr()), the QDMA > binding (qdma[1] -> qdma[0] in airoha_dev_set_qdma()), and the PPE cpu > port (CDM2 -> CDM1). Is that role change intentional, and could the > commit message call it out so bisecting a board that relied on the old > GDM3/GDM4=3DWAN semantics is easier? This change is intentional and this is currently not visible to the user since airoha_eth driver currently supports just the internal phy available via the MT7530 DSA switch and there are no WAN interfaces officially supported since PCS/external phy is not merged mainline yet (it is posted j= ust as RFC in [0]). I will add this info in the commimit log in v5. [0] https://lore.kernel.org/netdev/20260505182713.27644-1-ansuelsmth@gmail.= com/ >=20 > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/eth= ernet/airoha/airoha_eth.c > > index 1c4927c1aeb0..8617dd4f7932 100644 > > --- a/drivers/net/ethernet/airoha/airoha_eth.c > > +++ b/drivers/net/ethernet/airoha/airoha_eth.c > > @@ -1863,36 +1863,77 @@ static int airoha_set_gdm2_loopback(struct airo= ha_gdm_dev *dev) > > return 0; > > } > > =20 > > -static int airoha_dev_init(struct net_device *netdev) > > +static struct airoha_gdm_dev * > > +airoha_get_wan_gdm_dev(struct airoha_eth *eth) > > +{ > > + int i; > > + > > + for (i =3D 0; i < ARRAY_SIZE(eth->ports); i++) { > > + struct airoha_gdm_port *port =3D eth->ports[i]; > > + int j; > > + > > + if (!port) > > + continue; > > + > > + for (j =3D 0; j < ARRAY_SIZE(port->devs); j++) { > > + struct airoha_gdm_dev *dev =3D port->devs[j]; > > + > > + if (dev && !airoha_is_lan_gdm_dev(dev)) > > + return dev; > > + } > > + } > > + > > + return NULL; > > +} >=20 > [High] > airoha_register_gdm_devices() invokes airoha_dev_init() for each dev in > port->devs[] sequentially. On EN7581 GDM3 supports two devs (nbq 4/5 for > PCIE0/PCIE1 in airoha_alloc_gdm_device()). When both devs are registered, > devs[0] sets PRIV_FLAG_WAN and then devs[1] sees the first via > airoha_get_wan_gdm_dev() and falls through to LAN. Is that the intended > result for two devs on the same physical GDM3/GDM4 port? >=20 > The two devs then diverge: >=20 > airoha_dev_set_qdma() > dev->qdma =3D ð->qdma[!airoha_is_lan_gdm_dev(dev)]; >=20 > so devs[1] binds to qdma[0] while devs[0] is on qdma[1]; >=20 > airoha_set_macaddr() > reg =3D airoha_is_lan_gdm_dev(dev) ? REG_FE_LAN_MAC_H > : REG_FE_WAN_MAC_H; >=20 > so devs[1] writes into the LAN MAC range that belongs to GDM1; and in > airoha_ppe_foe_entry_prepare() pse_port becomes port->id instead of 2 > (GDM2 loopback) only for devs[1]. Is there an assumption that sibling > devs on the same port must share the WAN/LAN classification? A subsequent patch in the series adds suppport for multiple net_device that share the same LAN/WAN mac address register space. Moreover, I will post subsequent patches (not in the series) to dynamically set/get net_device LAN/WAN configuration. >=20 > > + > > +static void airoha_dev_set_qdma(struct airoha_gdm_dev *dev) > > { > > - struct airoha_gdm_dev *dev =3D netdev_priv(netdev); > > - struct airoha_gdm_port *port =3D dev->port; > > struct airoha_eth *eth =3D dev->eth; > > int i; > > =20 > > /* QDMA0 is used for lan ports while QDMA1 is used for WAN ports */ > > dev->qdma =3D ð->qdma[!airoha_is_lan_gdm_dev(dev)]; > > dev->dev->irq =3D dev->qdma->irq_banks[0].irq; > > - airoha_set_macaddr(dev, netdev->dev_addr); > > + > > + for (i =3D 0; i < eth->soc->num_ppe; i++) > > + airoha_ppe_set_cpu_port(dev, i, airoha_get_fe_port(dev)); > > +} > > + > > +static int airoha_dev_init(struct net_device *netdev) > > +{ > > + struct airoha_gdm_dev *dev =3D netdev_priv(netdev); > > + struct airoha_gdm_port *port =3D dev->port; > > =20 > > switch (port->id) { > > case AIROHA_GDM3_IDX: > > - case AIROHA_GDM4_IDX: > > - /* If GDM2 is active we can't enable loopback */ > > - if (!eth->ports[1]) { > > - int err; > > + case AIROHA_GDM4_IDX: { > > + struct airoha_eth *eth =3D dev->eth; > > =20 > > - err =3D airoha_set_gdm2_loopback(dev); > > - if (err) > > - return err; > > - } > > + if (eth->ports[1] || airoha_get_wan_gdm_dev(eth)) > > + break; >=20 > [Medium] > Is the eth->ports[1] check equivalent to asking whether a GDM2 dev > actually exists? airoha_alloc_gdm_port() assigns eth->ports[p] before > iterating the "airoha,eth-port" child nodes, so with all GDM2 child nodes > marked status=3D"disabled" and GDM3 or GDM4 enabled, eth->ports[1] is > non-NULL but port->devs[] is empty. In that case this branch still breaks > out of the switch and the uplink GDM3/GDM4 dev ends up LAN. Would checking > for an actual dev under eth->ports[1] be safer than checking the port > pointer alone? ack, I will fix it in v5. >=20 > [Medium, Low] > The policy that "just a single WAN one" is allowed is enforced by silently > breaking here when airoha_get_wan_gdm_dev() returns non-NULL. Would a > netdev_warn() or -EINVAL help an administrator notice that a second WAN > candidate was demoted to LAN? Also, with both GDM3 and GDM4 enabled and > GDM2 absent, the winner of PRIV_FLAG_WAN depends on eth->ports[] iteration > order and the port/dev allocation order; is there a deterministic, > documented way to choose which dev becomes WAN (DT property, for example)? We can't return -EINVAL here since we still need to run airoha_dev_set_qdma= () and airoha_set_macaddr(). Moreover, I will post subsequent patches (not in = the series) to dynamically set/get net_device LAN/WAN configuration. >=20 > > + fallthrough; > > + } > > + case AIROHA_GDM2_IDX: > > + /* GDM2 is always used as wan */ > > + dev->flags |=3D PRIV_FLAG_WAN; > > break; > > default: > > break; > > } > > =20 > > - for (i =3D 0; i < eth->soc->num_ppe; i++) > > - airoha_ppe_set_cpu_port(dev, i, airoha_get_fe_port(dev)); > > + airoha_dev_set_qdma(dev); > > + airoha_set_macaddr(dev, netdev->dev_addr); > > + > > + if (!airoha_is_lan_gdm_dev(dev) && > > + (port->id =3D=3D AIROHA_GDM3_IDX || port->id =3D=3D AIROHA_GDM4_I= DX)) { > > + int err; > > + > > + err =3D airoha_set_gdm2_loopback(dev); > > + if (err) { > > + dev->flags &=3D ~PRIV_FLAG_WAN; > > + return err; > > + } > > + } >=20 > [Medium] > On the error path, is clearing PRIV_FLAG_WAN sufficient? By the time > airoha_set_gdm2_loopback() returns a failure, airoha_dev_set_qdma() has > already pointed dev->qdma at ð->qdma[1] and set dev->dev->irq to the > WAN IRQ, airoha_set_macaddr() has written into REG_FE_WAN_MAC_H, > airoha_ppe_set_cpu_port() has programmed CDM2 as the PPE cpu port, and > airoha_set_gdm2_loopback() itself may have already programmed > REG_GDM_FWD_CFG(GDM2), REG_GDM_TXCHN_EN/RXCHN_EN, REG_GDM_LPBK_CFG, > REG_GDM_LEN_CFG, REG_FE_VIP_PORT_EN and REG_FE_IFC_PORT_EN before the > get_sport() branch failed. >=20 > After clearing the flag airoha_is_lan_gdm_dev(dev) reports LAN but > dev->qdma and the hardware registers above still describe the WAN setup. > Would it be cleaner to either unwind those steps or drop the flag clear? In this case net_device registration will fail and the pending hw configura= tion will not affect other running devices. I guess we can avoid to add unnecess= ary code complexity. >=20 > > =20 > > return 0; > > } > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/eth= ernet/airoha/airoha_eth.h > > index 3a313ac439e7..5715b03e630b 100644 > > --- a/drivers/net/ethernet/airoha/airoha_eth.h > > +++ b/drivers/net/ethernet/airoha/airoha_eth.h > > @@ -540,12 +540,17 @@ struct airoha_qdma { > > u64 fwd_tx_packets; > > }; > > =20 > > +enum airoha_priv_flags { > > + PRIV_FLAG_WAN =3D BIT(0), > > +}; >=20 > [Low] > This isn't a bug, but PRIV_FLAG_WAN and the u32 flags field below overlap > conceptually with struct net_device::priv_flags and the IFF_* namespace. > Would a driver-prefixed name such as AIROHA_DEV_F_WAN make the > driver-local scope obvious to future readers? ack, I will fix it in v5. Regards, Lorenzo >=20 > > + > > struct airoha_gdm_dev { > > struct airoha_gdm_port *port; > > struct airoha_qdma *qdma; > > struct airoha_eth *eth; > > struct net_device *dev; > > =20 > > + u32 flags; > > int nbq; > > }; >=20 > [ ... ] --cVtlXQ1B6cbL+hCZ Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCaf8BaAAKCRA6cBh0uS2t rJSeAP0QBnXEgLvI07F2C7GkDzkU+BlJ80LyIyCiwOmzBYCIZAEAtQDBtjNFBFVg QO9/RuVlx3Ku7yx9L95neIwOm3mJDws= =k9t2 -----END PGP SIGNATURE----- --cVtlXQ1B6cbL+hCZ--