| Message ID | 20240225-dm9601_ret_err-v1-1-02c1d959ea59@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: usb: dm9601: fix wrong return value in dm9601_mdio_read | expand |
On Sun, Feb 25, 2024 at 12:20:06AM +0100, Javier Carrasco wrote: > The MII code does not check the return value of mdio_read (among > others), and therefore no error code should be sent. A previous fix to > the use of an uninitialized variable propagates negative error codes, > that might lead to wrong operations by the MII library. > > An example of such issues is the use of mii_nway_restart by the dm9601 > driver. The mii_nway_restart function does not check the value returned > by mdio_read, which in this case might be a negative number which could > contain the exact bit the function checks (BMCR_ANENABLE = 0x1000). > > Return zero in case of error, as it is common practice in users of > mdio_read to avoid wrong uses of the return value. > > Fixes: 8f8abb863fa5 ("net: usb: dm9601: fix uninitialized variable use in dm9601_mdio_read") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> I guess it would be nice if error values could be used, but as you have described, that does not seem to be the case here. Reviewed-by: Simon Horman <horms@kernel.org>
>>>>> "Javier" == Javier Carrasco <javier.carrasco.cruz@gmail.com> writes: > The MII code does not check the return value of mdio_read (among > others), and therefore no error code should be sent. A previous fix to > the use of an uninitialized variable propagates negative error codes, > that might lead to wrong operations by the MII library. > An example of such issues is the use of mii_nway_restart by the dm9601 > driver. The mii_nway_restart function does not check the value returned > by mdio_read, which in this case might be a negative number which could > contain the exact bit the function checks (BMCR_ANENABLE = 0x1000). > Return zero in case of error, as it is common practice in users of > mdio_read to avoid wrong uses of the return value. > Fixes: 8f8abb863fa5 ("net: usb: dm9601: fix uninitialized variable use in dm9601_mdio_read") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Reviewed-by: Peter Korsgaard <peter@korsgaard.com>
On Sun, 25 Feb 2024 00:20:06 +0100 Javier Carrasco wrote: > The MII code does not check the return value of mdio_read (among > others), and therefore no error code should be sent. A previous fix to > the use of an uninitialized variable propagates negative error codes, > that might lead to wrong operations by the MII library. > > An example of such issues is the use of mii_nway_restart by the dm9601 > driver. The mii_nway_restart function does not check the value returned > by mdio_read, which in this case might be a negative number which could > contain the exact bit the function checks (BMCR_ANENABLE = 0x1000). > > Return zero in case of error, as it is common practice in users of > mdio_read to avoid wrong uses of the return value. A bit odd but appears to be true, so I'll apply, thank you! Andrew, mii.h files seem to fall under PHYLIB in MAINTAINERS, but mii.c does not. Is this intentional?
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Sun, 25 Feb 2024 00:20:06 +0100 you wrote: > The MII code does not check the return value of mdio_read (among > others), and therefore no error code should be sent. A previous fix to > the use of an uninitialized variable propagates negative error codes, > that might lead to wrong operations by the MII library. > > An example of such issues is the use of mii_nway_restart by the dm9601 > driver. The mii_nway_restart function does not check the value returned > by mdio_read, which in this case might be a negative number which could > contain the exact bit the function checks (BMCR_ANENABLE = 0x1000). > > [...] Here is the summary with links: - net: usb: dm9601: fix wrong return value in dm9601_mdio_read https://git.kernel.org/netdev/net/c/c68b2c9eba38 You are awesome, thank you!
> Andrew, > mii.h files seem to fall under PHYLIB in MAINTAINERS, but mii.c does > not. Is this intentional? Probably. There are big parts of linux/mii.h which phylib uses. However drivers/net/mii.c is not part of phylib and is unmaintained. Andrew
diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c index 99ec1d4a972d..8b6d6a1b3c2e 100644 --- a/drivers/net/usb/dm9601.c +++ b/drivers/net/usb/dm9601.c @@ -232,7 +232,7 @@ static int dm9601_mdio_read(struct net_device *netdev, int phy_id, int loc) err = dm_read_shared_word(dev, 1, loc, &res); if (err < 0) { netdev_err(dev->net, "MDIO read error: %d\n", err); - return err; + return 0; } netdev_dbg(dev->net,
The MII code does not check the return value of mdio_read (among others), and therefore no error code should be sent. A previous fix to the use of an uninitialized variable propagates negative error codes, that might lead to wrong operations by the MII library. An example of such issues is the use of mii_nway_restart by the dm9601 driver. The mii_nway_restart function does not check the value returned by mdio_read, which in this case might be a negative number which could contain the exact bit the function checks (BMCR_ANENABLE = 0x1000). Return zero in case of error, as it is common practice in users of mdio_read to avoid wrong uses of the return value. Fixes: 8f8abb863fa5 ("net: usb: dm9601: fix uninitialized variable use in dm9601_mdio_read") Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/net/usb/dm9601.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240224-dm9601_ret_err-0a9c9d95cd10 Best regards,