| Message ID | 20240314063008.11214-1-adiupina@astralinux.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: add return value check of genphy_read_status() | expand |
On Thu, Mar 14, 2024 at 09:30:08AM +0300, Alexandra Diupina wrote: > Need to check return value of genphy_read_status(), > because higher in the call hierarchy is the > dsa_register_switch() function, > which is used in various drivers. I don't understand the commit message. Why is it important to dsa_register_switch()? Andrew
Hello, Andrew! The dsa_register_switch() function is used in various DSA drivers (in probe function), so it is necessary to check all possible errors. If the return value (may be an error code) of genphy_read_status() is not checked in dsa_shared_port_fixed_link_register_of(), a possible error in dsa_register_switch() may be missed. 14/03/24 16:02, Andrew Lunn пишет: > On Thu, Mar 14, 2024 at 09:30:08AM +0300, Alexandra Diupina wrote: >> Need to check return value of genphy_read_status(), >> because higher in the call hierarchy is the >> dsa_register_switch() function, >> which is used in various drivers. > I don't understand the commit message. Why is it important to > dsa_register_switch()? > > Andrew
On 15/03/2024 03:25, Александра Дюпина wrote: > Hello, Andrew! (please do not top-post) > > The dsa_register_switch() function is used in various DSA > drivers (in probe function), so it is necessary to check all > possible errors. If the return value (may be an error code) > of genphy_read_status() is not checked in > dsa_shared_port_fixed_link_register_of(), a possible error > in dsa_register_switch() may be missed. This is not a path that will fail, because the fixed PHY emulation layer is not a real piece of hardware, therefore no MDIO read could really cause a problem here. I don't have a strong opinion however if you want to propagate it properly.
21/03/24 02:48, Florian Fainelli пишет: > > > On 15/03/2024 03:25, Александра Дюпина wrote: >> Hello, Andrew! > > (please do not top-post) > >> >> The dsa_register_switch() function is used in various DSA >> drivers (in probe function), so it is necessary to check all >> possible errors. If the return value (may be an error code) >> of genphy_read_status() is not checked in >> dsa_shared_port_fixed_link_register_of(), a possible error >> in dsa_register_switch() may be missed. > > This is not a path that will fail, because the fixed PHY emulation > layer is not a real piece of hardware, therefore no MDIO read could > really cause a problem here. I don't have a strong opinion however if > you want to propagate it properly. Hi, Florian! I would like to make sure that I have understood you correctly. Checking the return value of genphy_read_status() in dsa_shared_port_fixed_link_register_of() is not needed because dsa_shared_port_fixed_link_register_of() is called if of_phy_is_fixed_link()==true (this means that the PHY emulation layer is used, link is registered by of_phy_register_fixed_link() without errors and therefore there cannot be an error in genphy_read_status()). Right?
just a friendly reminder Alexandra
diff --git a/net/dsa/port.c b/net/dsa/port.c index c42dac87671b..c411f30bb5f6 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1765,7 +1765,9 @@ static int dsa_shared_port_fixed_link_register_of(struct dsa_port *dp) mode = PHY_INTERFACE_MODE_NA; phydev->interface = mode; - genphy_read_status(phydev); + err = genphy_read_status(phydev); + if (err) + return err; if (ds->ops->adjust_link) ds->ops->adjust_link(ds, port, phydev);
Need to check return value of genphy_read_status(), because higher in the call hierarchy is the dsa_register_switch() function, which is used in various drivers. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 33615367f378 ("net: dsa: Support internal phy on 'cpu' port") Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru> --- net/dsa/port.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)