[PATCH] net: phy: fix phy_read_poll_timeout argument type in genphy_loopback

[PATCH] net: phy: fix phy_read_poll_timeout argument type in genphy_loopback

[Nikita Kiryushin]

read_poll_timeout inside phy_read_poll_timeout can set val negative
in some cases (for example, __mdiobus_read inside phy_read can return
-EOPNOTSUPP).

Supposedly, commit 4ec732951702 ("net: phylib: fix 
phy_read*_poll_timeout()")
should fix problems with wrong-signed vals, but I do not see how
as val is sent to phy_read as is and __val = phy_read (not val)
is checked for sign.

Change val type for signed to allow better error handling as done in other
phy_read_poll_timeout callers. This will not fix any error handling
by itself, but allows, for example, to modify cond with appropriate
sign check or check resulting val separately.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 014068dcb5b1 ("net: phy: genphy_loopback: add link speed 
configuration")
Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
---
  drivers/net/phy/phy_device.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8297ef681bf5..6c6ec9475709 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2831,8 +2831,8 @@ EXPORT_SYMBOL(genphy_resume);
  int genphy_loopback(struct phy_device *phydev, bool enable)
  {
  	if (enable) {
-		u16 val, ctl = BMCR_LOOPBACK;
-		int ret;
+		u16 ctl = BMCR_LOOPBACK;
+		int ret, val;
   		ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
  -- 2.34.1

Re: [PATCH] net: phy: fix phy_read_poll_timeout argument type in genphy_loopback

[Russell King (Oracle)]

On Wed, Mar 13, 2024 at 09:13:23PM +0300, Nikita Kiryushin wrote:
> read_poll_timeout inside phy_read_poll_timeout can set val negative
> in some cases (for example, __mdiobus_read inside phy_read can return
> -EOPNOTSUPP).
> 
> Supposedly, commit 4ec732951702 ("net: phylib: fix
> phy_read*_poll_timeout()")
> should fix problems with wrong-signed vals, but I do not see how
> as val is sent to phy_read as is and __val = phy_read (not val)
> is checked for sign.
> 
> Change val type for signed to allow better error handling as done in other
> phy_read_poll_timeout callers. This will not fix any error handling
> by itself, but allows, for example, to modify cond with appropriate
> sign check or check resulting val separately.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 014068dcb5b1 ("net: phy: genphy_loopback: add link speed
> configuration")

Fixes lines must not be wrapped.

> Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
> ---
>  drivers/net/phy/phy_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8297ef681bf5..6c6ec9475709 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2831,8 +2831,8 @@ EXPORT_SYMBOL(genphy_resume);
>  int genphy_loopback(struct phy_device *phydev, bool enable)
>  {
>  	if (enable) {
> -		u16 val, ctl = BMCR_LOOPBACK;
> -		int ret;
> +		u16 ctl = BMCR_LOOPBACK;
> +		int ret, val;
>   		ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
>  -- 2.34.1

The patch seems broken - blank lines appear to be missing.

I think the change itself is correct however - "val" passed into
phy_read_poll_timeout() must be signed.

Lastly, as you need to indicate which tree your patch is targetting,
which is done via the subject line prefix. As this is in mainline, then
[PATCH net] would be the correct prefix.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!