* Re: [PATCH 18/18] net: dsa: microchip: Check return value of of_get_phy_mode()
2024-02-11 15:08 [PATCH 18/18] net: dsa: microchip: Check return value of of_get_phy_mode() Daniil Dulov
@ 2024-02-13 0:50 ` Jakub Kicinski
2024-02-13 3:19 ` Arun.Ramadoss
2024-02-16 1:09 ` Vladimir Oltean
2 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2024-02-13 0:50 UTC (permalink / raw)
To: Daniil Dulov
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller,
Alexandre Belloni, Helmut Grohne, netdev, linux-kernel,
lvc-project
On Sun, 11 Feb 2024 07:08:04 -0800 Daniil Dulov wrote:
> + ret = of_get_phy_mode(port,
> &dev->ports[port_num].interface);
1) run checkpatch please.
2) we only got patch 18 on netdev, and we don't process partial series.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 18/18] net: dsa: microchip: Check return value of of_get_phy_mode()
2024-02-11 15:08 [PATCH 18/18] net: dsa: microchip: Check return value of of_get_phy_mode() Daniil Dulov
2024-02-13 0:50 ` Jakub Kicinski
@ 2024-02-13 3:19 ` Arun.Ramadoss
2024-02-16 1:09 ` Vladimir Oltean
2 siblings, 0 replies; 4+ messages in thread
From: Arun.Ramadoss @ 2024-02-13 3:19 UTC (permalink / raw)
To: Woojung.Huh, d.dulov
Cc: andrew, olteanv, davem, vivien.didelot, helmut.grohne,
linux-kernel, f.fainelli, alexandre.belloni, UNGLinuxDriver, kuba,
lvc-project, netdev
Hi Daniil,
On Sun, 2024-02-11 at 07:08 -0800, Daniil Dulov wrote:
> drivers/net/dsa/microchip/ksz_common.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index e3c338624b95..faffce6ea1e1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -434,8 +434,10 @@ int ksz_switch_register(struct ksz_device *dev,
> continue;
> if (port_num >= dev->mib_port_cnt)
> return -EINVAL;
> - of_get_phy_mode(port,
> + ret = of_get_phy_mode(port,
> &dev-
> >ports[port_num].interface);
> + if (ret)
> + return ret;
If phy mode is not defined in device tree, can phy mode have some
default mode like internal or rgmii_tx_id.
If this field is mandatory to be specified in device tree, Then instead
of just returning, it will be good to return with dev_err message, so
the user will be aware of whats wrong with the driver.
> }
> dev->synclko_125 = of_property_read_bool(dev->dev-
> >of_node,
> "microchip,s
> ynclko-125");
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 18/18] net: dsa: microchip: Check return value of of_get_phy_mode()
2024-02-11 15:08 [PATCH 18/18] net: dsa: microchip: Check return value of of_get_phy_mode() Daniil Dulov
2024-02-13 0:50 ` Jakub Kicinski
2024-02-13 3:19 ` Arun.Ramadoss
@ 2024-02-16 1:09 ` Vladimir Oltean
2 siblings, 0 replies; 4+ messages in thread
From: Vladimir Oltean @ 2024-02-16 1:09 UTC (permalink / raw)
To: Daniil Dulov
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, David S. Miller, Jakub Kicinski,
Alexandre Belloni, Helmut Grohne, netdev, linux-kernel,
lvc-project
Hi Daniil,
On Sun, Feb 11, 2024 at 07:08:04AM -0800, Daniil Dulov wrote:
> of_get_phy_mode() may return an error, so add a check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: edecfa98f602 ("net: dsa: microchip: look for phy-mode in port nodes")
> Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru>
> ---
> drivers/net/dsa/microchip/ksz_common.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index e3c338624b95..faffce6ea1e1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -434,8 +434,10 @@ int ksz_switch_register(struct ksz_device *dev,
> continue;
> if (port_num >= dev->mib_port_cnt)
> return -EINVAL;
> - of_get_phy_mode(port,
> + ret = of_get_phy_mode(port,
> &dev->ports[port_num].interface);
> + if (ret)
> + return ret;
> }
> dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
> "microchip,synclko-125");
> --
> 2.25.1
>
Not all functions that may return an error, and that error is ignored,
must be fixed.
Here, if of_get_phy_mode() fails, dev->ports[port_num].interface remains
at its default value (0, or PHY_INTERFACE_MODE_NA). It is a valid value
to the rest of the driver at least in some cases (user ports connected
to internal PHYs). And since device tree is ABI, you are proposing to
break something that may have used to work, by making a previously well
treated path now cause an error.
Sorry, NACK even as a matter of principle, do not even bother to resend this
without a more careful analysis.
^ permalink raw reply [flat|nested] 4+ messages in thread