| Message ID | 20240606085234.565551-6-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: lantiq_gswip: code improvements | expand |
On Thu, Jun 06, 2024 at 10:52:26AM +0200, Martin Schiller wrote: > Before commit 74be4babe72f ("net: dsa: do not enable or disable non user > ports"), gswip_port_enable/disable() were also executed for the cpu port > in gswip_setup() which disabled the cpu port during initialization. Ah, you also noticed this. > > Let's restore this by removing the dsa_is_user_port checks. Also, let's > clean up the gswip_port_enable() function so that we only have to check > for the cpu port once. > > Fixes: 74be4babe72f ("net: dsa: do not enable or disable non user ports") Fixes tags shouldn't be taken lightly. If you think there's a functional user-visible problem caused by that change, you need to explain what that problem is and what it affects. Additionally, bug fix patches are sent out to the 'net' tree, not bundled up with 'net-next' material (unless they fix a change that's also exclusive to net-next). Otherwise, just drop the 'Fixes' tag. > Signed-off-by: Martin Schiller <ms@dev.tdt.de> > --- > drivers/net/dsa/lantiq_gswip.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c > index 3fd5599fca52..38b5f743e5ee 100644 > --- a/drivers/net/dsa/lantiq_gswip.c > +++ b/drivers/net/dsa/lantiq_gswip.c > @@ -695,13 +695,18 @@ static int gswip_port_enable(struct dsa_switch *ds, int port, > struct gswip_priv *priv = ds->priv; > int err; > > - if (!dsa_is_user_port(ds, port)) > - return 0; > - > if (!dsa_is_cpu_port(ds, port)) { > + u32 mdio_phy = 0; > + > err = gswip_add_single_port_br(priv, port, true); > if (err) > return err; > + > + if (phydev) > + mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK; > + > + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy, > + GSWIP_MDIO_PHYp(port)); > } > > /* RMON Counter Enable for port */ > @@ -714,16 +719,6 @@ static int gswip_port_enable(struct dsa_switch *ds, int port, > gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN, > GSWIP_SDMA_PCTRLp(port)); > > - if (!dsa_is_cpu_port(ds, port)) { > - u32 mdio_phy = 0; > - > - if (phydev) > - mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK; > - > - gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy, > - GSWIP_MDIO_PHYp(port)); > - } > - > return 0; > } It would be good to state in the commit message that the operation reordering is safe. The commit seems to be concerned mainly with code cleanliness, which does not always take side effects into account.
On 2024-06-07 13:18, Vladimir Oltean wrote: > On Thu, Jun 06, 2024 at 10:52:26AM +0200, Martin Schiller wrote: >> Before commit 74be4babe72f ("net: dsa: do not enable or disable non >> user >> ports"), gswip_port_enable/disable() were also executed for the cpu >> port >> in gswip_setup() which disabled the cpu port during initialization. > > Ah, you also noticed this. > >> >> Let's restore this by removing the dsa_is_user_port checks. Also, >> let's >> clean up the gswip_port_enable() function so that we only have to >> check >> for the cpu port once. >> >> Fixes: 74be4babe72f ("net: dsa: do not enable or disable non user >> ports") > > Fixes tags shouldn't be taken lightly. If you think there's a > functional > user-visible problem caused by that change, you need to explain what > that problem is and what it affects. Additionally, bug fix patches are > sent out to the 'net' tree, not bundled up with 'net-next' material > (unless they fix a change that's also exclusive to net-next). > Otherwise, just drop the 'Fixes' tag. OK, I will drop the 'Fixes' tag. > >> Signed-off-by: Martin Schiller <ms@dev.tdt.de> >> --- >> drivers/net/dsa/lantiq_gswip.c | 24 ++++++++---------------- >> 1 file changed, 8 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/dsa/lantiq_gswip.c >> b/drivers/net/dsa/lantiq_gswip.c >> index 3fd5599fca52..38b5f743e5ee 100644 >> --- a/drivers/net/dsa/lantiq_gswip.c >> +++ b/drivers/net/dsa/lantiq_gswip.c >> @@ -695,13 +695,18 @@ static int gswip_port_enable(struct dsa_switch >> *ds, int port, >> struct gswip_priv *priv = ds->priv; >> int err; >> >> - if (!dsa_is_user_port(ds, port)) >> - return 0; >> - >> if (!dsa_is_cpu_port(ds, port)) { >> + u32 mdio_phy = 0; >> + >> err = gswip_add_single_port_br(priv, port, true); >> if (err) >> return err; >> + >> + if (phydev) >> + mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK; >> + >> + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy, >> + GSWIP_MDIO_PHYp(port)); >> } >> >> /* RMON Counter Enable for port */ >> @@ -714,16 +719,6 @@ static int gswip_port_enable(struct dsa_switch >> *ds, int port, >> gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN, >> GSWIP_SDMA_PCTRLp(port)); >> >> - if (!dsa_is_cpu_port(ds, port)) { >> - u32 mdio_phy = 0; >> - >> - if (phydev) >> - mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK; >> - >> - gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy, >> - GSWIP_MDIO_PHYp(port)); >> - } >> - >> return 0; >> } > > It would be good to state in the commit message that the operation > reordering is safe. The commit seems to be concerned mainly with code > cleanliness, which does not always take side effects into account. Thanks for the hint. I will take it into account in the commit message.
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c index 3fd5599fca52..38b5f743e5ee 100644 --- a/drivers/net/dsa/lantiq_gswip.c +++ b/drivers/net/dsa/lantiq_gswip.c @@ -695,13 +695,18 @@ static int gswip_port_enable(struct dsa_switch *ds, int port, struct gswip_priv *priv = ds->priv; int err; - if (!dsa_is_user_port(ds, port)) - return 0; - if (!dsa_is_cpu_port(ds, port)) { + u32 mdio_phy = 0; + err = gswip_add_single_port_br(priv, port, true); if (err) return err; + + if (phydev) + mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK; + + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy, + GSWIP_MDIO_PHYp(port)); } /* RMON Counter Enable for port */ @@ -714,16 +719,6 @@ static int gswip_port_enable(struct dsa_switch *ds, int port, gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN, GSWIP_SDMA_PCTRLp(port)); - if (!dsa_is_cpu_port(ds, port)) { - u32 mdio_phy = 0; - - if (phydev) - mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK; - - gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy, - GSWIP_MDIO_PHYp(port)); - } - return 0; } @@ -731,9 +726,6 @@ static void gswip_port_disable(struct dsa_switch *ds, int port) { struct gswip_priv *priv = ds->priv; - if (!dsa_is_user_port(ds, port)) - return; - gswip_switch_mask(priv, GSWIP_FDMA_PCTRL_EN, 0, GSWIP_FDMA_PCTRLp(port)); gswip_switch_mask(priv, GSWIP_SDMA_PCTRL_EN, 0,
Before commit 74be4babe72f ("net: dsa: do not enable or disable non user ports"), gswip_port_enable/disable() were also executed for the cpu port in gswip_setup() which disabled the cpu port during initialization. Let's restore this by removing the dsa_is_user_port checks. Also, let's clean up the gswip_port_enable() function so that we only have to check for the cpu port once. Fixes: 74be4babe72f ("net: dsa: do not enable or disable non user ports") Signed-off-by: Martin Schiller <ms@dev.tdt.de> --- drivers/net/dsa/lantiq_gswip.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)