| Message ID | ff2a5af67361988b3581831f7bd1eddebfb4c48f.1712082763.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c120209bce34c49dcaba32f15679574327d09f63 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: sja1105: Fix parameters order in sja1110_pcs_mdio_write_c45() | expand |
On Tue Apr 2, 2024 at 8:33 PM CEST, Christophe JAILLET wrote: > The definition and declaration of sja1110_pcs_mdio_write_c45() don't have > parameters in the same order. > > Knowing that sja1110_pcs_mdio_write_c45() is used as a function pointer > in 'sja1105_info' structure with .pcs_mdio_write_c45, and that we have: > > int (*pcs_mdio_write_c45)(struct mii_bus *bus, int phy, int mmd, > int reg, u16 val); > > it is likely that the definition is the one to change. See also "struct mii_bus": /** @write_c45: Perform a C45 write transfer on the bus */ int (*write_c45)(struct mii_bus *bus, int addr, int devnum, int regnum, u16 val); > > Found with cppcheck, funcArgOrderDifferent. > > Fixes: ae271547bba6 ("net: dsa: sja1105: C45 only transactions for PCS") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only. > --- > drivers/net/dsa/sja1105/sja1105_mdio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c > index 833e55e4b961..52ddb4ef259e 100644 > --- a/drivers/net/dsa/sja1105/sja1105_mdio.c > +++ b/drivers/net/dsa/sja1105/sja1105_mdio.c > @@ -94,7 +94,7 @@ int sja1110_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg) > return tmp & 0xffff; > } > > -int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int reg, int mmd, > +int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg, > u16 val) Reviewed-by: Michael Walle <mwalle@kernel.org> Vladimir, do you happen to know if some of your boards will use this function? Just wondering because it was never noticed. -michael > { > struct sja1105_mdio_private *mdio_priv = bus->priv;
On Wed, Apr 03, 2024 at 10:06:51AM +0200, Michael Walle wrote: > Vladimir, do you happen to know if some of your boards will use this > function? Just wondering because it was never noticed. > > -michael The SJA1110 uses the XPCS only for SGMII and 2500Base-X links. On the Bluebox3 I have (which is currently in a cardboard box for the time being, so I will have to rely on static analysis just like everybody else), these links are the cascade ports between switches. However, those cascade ports are only used for autonomous traffic bridging (the board has a weird "H" topology). Traffic terminated on the CPU doesn't go through the SGMII links, so this is why I haven't noticed it during casual testing. However, there are other NXP systems using downstream device trees which are likely impacted, but they are on older kernels and probably haven't seen the regression just yet. So the fix is welcome.
On Tue, Apr 02, 2024 at 08:33:56PM +0200, Christophe JAILLET wrote: > The definition and declaration of sja1110_pcs_mdio_write_c45() don't have > parameters in the same order. > > Knowing that sja1110_pcs_mdio_write_c45() is used as a function pointer > in 'sja1105_info' structure with .pcs_mdio_write_c45, and that we have: > > int (*pcs_mdio_write_c45)(struct mii_bus *bus, int phy, int mmd, > int reg, u16 val); > > it is likely that the definition is the one to change. > > Found with cppcheck, funcArgOrderDifferent. > > Fixes: ae271547bba6 ("net: dsa: sja1105: C45 only transactions for PCS") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only. > --- Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 2 Apr 2024 20:33:56 +0200 you wrote: > The definition and declaration of sja1110_pcs_mdio_write_c45() don't have > parameters in the same order. > > Knowing that sja1110_pcs_mdio_write_c45() is used as a function pointer > in 'sja1105_info' structure with .pcs_mdio_write_c45, and that we have: > > int (*pcs_mdio_write_c45)(struct mii_bus *bus, int phy, int mmd, > int reg, u16 val); > > [...] Here is the summary with links: - [net] net: dsa: sja1105: Fix parameters order in sja1110_pcs_mdio_write_c45() https://git.kernel.org/netdev/net/c/c120209bce34 You are awesome, thank you!
diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c index 833e55e4b961..52ddb4ef259e 100644 --- a/drivers/net/dsa/sja1105/sja1105_mdio.c +++ b/drivers/net/dsa/sja1105/sja1105_mdio.c @@ -94,7 +94,7 @@ int sja1110_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg) return tmp & 0xffff; } -int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int reg, int mmd, +int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg, u16 val) { struct sja1105_mdio_private *mdio_priv = bus->priv;
The definition and declaration of sja1110_pcs_mdio_write_c45() don't have parameters in the same order. Knowing that sja1110_pcs_mdio_write_c45() is used as a function pointer in 'sja1105_info' structure with .pcs_mdio_write_c45, and that we have: int (*pcs_mdio_write_c45)(struct mii_bus *bus, int phy, int mmd, int reg, u16 val); it is likely that the definition is the one to change. Found with cppcheck, funcArgOrderDifferent. Fixes: ae271547bba6 ("net: dsa: sja1105: C45 only transactions for PCS") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only. --- drivers/net/dsa/sja1105/sja1105_mdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)