Messages in this thread | | | Date | Wed, 13 Mar 2024 19:12:59 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH] net: phy: fix phy_read_poll_timeout argument type in genphy_loopback |
| |
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!
| |