Calling __phy_read() might return a negative error code. Use 'int'
to declare variables which call __phy_read() and also add error check
for them.

The numerous callers of vsc8584_macsec_phy_read() don't expect it to
fail. So don't return the error code from __phy_read(), but also don't
return random values if it does fail.

Fixes: fa164e40c53b ("net: phy: mscc: split the driver into separate files")
Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
Changelog:
v2:
- Sort variable declaration and add a detailed comment.
---
 drivers/net/phy/mscc/mscc_macsec.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_macsec.c b/drivers/net/phy/mscc/mscc_macsec.c
index b7b2521c73fb..58ad11a697b6 100644
--- a/drivers/net/phy/mscc/mscc_macsec.c
+++ b/drivers/net/phy/mscc/mscc_macsec.c
@@ -22,9 +22,9 @@
 static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
 				   enum macsec_bank bank, u32 reg)
 {
-	u32 val, val_l = 0, val_h = 0;
+	int rc, val, val_l, val_h;
 	unsigned long deadline;
-	int rc;
+	u32 ret = 0;
 
 	rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
 	if (rc < 0)
@@ -47,15 +47,20 @@ static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
 	deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
 	do {
 		val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19);
+		if (val < 0)
+			goto failed;
 	} while (time_before(jiffies, deadline) && !(val & MSCC_PHY_MACSEC_19_CMD));
 
 	val_l = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_17);
 	val_h = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_18);
 
+	if (val_l > 0 && val_h > 0)
+		ret = (val_h << 16) | val_l;
+
 failed:
 	phy_restore_page(phydev, rc, rc);
 
-	return (val_h << 16) | val_l;
+	return ret;
 }
 
 static void vsc8584_macsec_phy_write(struct phy_device *phydev,
-- 
2.35.1

On Tue, May 10, 2022 at 10:22:45PM +0800, Wan Jiabing wrote:
> Calling __phy_read() might return a negative error code. Use 'int'
> to declare variables which call __phy_read() and also add error check
> for them.
> 
> The numerous callers of vsc8584_macsec_phy_read() don't expect it to
> fail. So don't return the error code from __phy_read(), but also don't
> return random values if it does fail.
> 
> Fixes: fa164e40c53b ("net: phy: mscc: split the driver into separate files")
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> Changelog:
> v2:
> - Sort variable declaration and add a detailed comment.
> ---
>  drivers/net/phy/mscc/mscc_macsec.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/mscc/mscc_macsec.c b/drivers/net/phy/mscc/mscc_macsec.c
> index b7b2521c73fb..58ad11a697b6 100644
> --- a/drivers/net/phy/mscc/mscc_macsec.c
> +++ b/drivers/net/phy/mscc/mscc_macsec.c
> @@ -22,9 +22,9 @@
>  static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
>  				   enum macsec_bank bank, u32 reg)
>  {
> -	u32 val, val_l = 0, val_h = 0;
> +	int rc, val, val_l, val_h;
>  	unsigned long deadline;
> -	int rc;
> +	u32 ret = 0;
>  
>  	rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
>  	if (rc < 0)
> @@ -47,15 +47,20 @@ static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
>  	deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
>  	do {
>  		val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19);
> +		if (val < 0)
> +			goto failed;
>  	} while (time_before(jiffies, deadline) && !(val & MSCC_PHY_MACSEC_19_CMD));
>  
>  	val_l = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_17);
>  	val_h = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_18);
>  
> +	if (val_l > 0 && val_h > 0)
> +		ret = (val_h << 16) | val_l;
> +
>  failed:
>  	phy_restore_page(phydev, rc, rc);
>  
> -	return (val_h << 16) | val_l;
> +	return ret;
>  }

This is still wrong - phy_restore_page() can fail to retore the page.

It's rather unfortunate that you need to return a u32, where the
high values become negative ints, which means you can't use
phy_restore_page() as it's supposed to be used.

If you fail to read from the PHY, is returning zero acceptable?

I think what you should be doing at the very least is:

	rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
	if (rc < 0)
		goto failed;

	rc = __phy_write(phydev, MSCC_EXT_PAGE_MACSEC_20, ...);
	if (rc < 0)
		goto failed;

	...

	rc = __phy_write(phydev, MSCC_EXT_PAGE_MACSEC_19, ...);
	if (rc < 0)
		goto failed;

	...
	do {
		val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19);
		if (val < 0) {
			rc = val;
			goto failed;
		}
	} while (...);

	val_l = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_17);
	if (val_l < 0) {
		rc = val_l;
		goto failed;
	}

	val_h = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_18);
	if (val_h < 0)
		rc = val_h;

failed:
	rc = phy_restore_page(phgydev, rc, 0);

	return rc < 0 ? 0 : val_h << 16 | val_l;

Which means that if any of the PHY IO functions fail at any point, this
returns zero.

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

Hello,

Quoting Wan Jiabing (2022-05-10 16:22:45)
> Calling __phy_read() might return a negative error code. Use 'int'
> to declare variables which call __phy_read() and also add error check
> for them.
> 
> The numerous callers of vsc8584_macsec_phy_read() don't expect it to
> fail. So don't return the error code from __phy_read(), but also don't
> return random values if it does fail.
> 
> Fixes: fa164e40c53b ("net: phy: mscc: split the driver into separate files")

Does this fix an actual issue or was this found by code inspection? If
that is not fixing a real issue I don't think it should go to stable
trees.

Also this is not the right commit, the __phy_read call was introduced
before splitting the file.

>  static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
>                                    enum macsec_bank bank, u32 reg)
>  {
> -       u32 val, val_l = 0, val_h = 0;
> +       int rc, val, val_l, val_h;
>         unsigned long deadline;
> -       int rc;
> +       u32 ret = 0;
>  
>         rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
>         if (rc < 0)
> @@ -47,15 +47,20 @@ static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
>         deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
>         do {
>                 val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19);
> +               if (val < 0)
> +                       goto failed;
>         } while (time_before(jiffies, deadline) && !(val & MSCC_PHY_MACSEC_19_CMD));
>  
>         val_l = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_17);
>         val_h = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_18);
>  
> +       if (val_l > 0 && val_h > 0)
> +               ret = (val_h << 16) | val_l;

Both values have to be non-0 for the function to return a value? I
haven't checked but I would assume it is valid to have one of the two
being 0.

>  failed:
>         phy_restore_page(phydev, rc, rc);
>  
> -       return (val_h << 16) | val_l;
> +       return ret;
>  }

Thanks,
Antoine

> Does this fix an actual issue or was this found by code inspection? If
> that is not fixing a real issue I don't think it should go to stable
> trees.

You are probably right about stable vs net-next. With the old code, a
bad read will result in random return values and bad things are likely
to happen. With this change, 0 will be returned, and hopefully less
bad things will happen.

But i doubt this impacts real users. MDIO tends to either work or not
work at all. And not working is pretty noticeable, and nobody has
reported issues.

So, lets drop the fixes tag, and submit to net-next.

	 Andrew

Quoting Andrew Lunn (2022-05-10 17:08:07)
> 
> But i doubt this impacts real users. MDIO tends to either work or not
> work at all. And not working is pretty noticeable, and nobody has
> reported issues.

Right. On top of that there are other calls to __phy_read in this driver
not checking the returned value. Plus all __phy_write calls. If that was
found by code inspection I would suggest to improve the whole driver and
not this function alone.

Thanks,
Antoine

On 2022/5/10 23:33, Antoine Tenart wrote:
> Quoting Andrew Lunn (2022-05-10 17:08:07)
>> But i doubt this impacts real users. MDIO tends to either work or not
>> work at all. And not working is pretty noticeable, and nobody has
>> reported issues.
> Right. On top of that there are other calls to __phy_read in this driver
> not checking the returned value. Plus all __phy_write calls. If that was
> found by code inspection I would suggest to improve the whole driver and
> not this function alone.
OK, I'll try to improve the whole driver and send a patch set for this.

Thanks,
Wan Jiabing