It would be better to replace the traditional ternary conditional
operator with min()

Signed-off-by: Lu Hongfei <luhongfei@vivo.com>
---
 drivers/net/phy/phy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 drivers/net/phy/phy.c

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0c0df38cd1ab..a8beb4ab8451
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1002,7 +1002,7 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
 	if (!ret)
 		return -ETIMEDOUT;
 
-	return ret < 0 ? ret : 0;
+	return min(ret, 0);
 }
 
 int phy_ethtool_ksettings_set(struct phy_device *phydev,
@@ -1526,7 +1526,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
 				       MDIO_PCS_CTRL1_CLKSTOP_EN);
 
-	return ret < 0 ? ret : 0;
+	return min(ret, 0);
 }
 EXPORT_SYMBOL(phy_init_eee);
 
-- 
2.39.0

On Tue, May 30, 2023 at 04:45:30PM +0800, Lu Hongfei wrote:
> It would be better to replace the traditional ternary conditional
> operator with min()

Hi Lu

Adding to the comments from Russell and Heiner, if i remember
correctly, the exact same change has been rejected before, for the
same reasons.  When submitting a patch, please do a search first and
see if somebody else has already received a reject.

Did you use a static analyser to find this? Please submit a patch to
the static analyser to stop it reporting code like this. That will
save wasting peoples time having to develop such bad patches, and
reviewers having to reject them.

	Andrew

On Tue, May 30, 2023 at 04:45:30PM +0800, Lu Hongfei wrote:
> It would be better to replace the traditional ternary conditional
> operator with min()

I don't think this is any "better". It's not really a "let's return the
minimum of two values" even though that is what it ends up functionally
being.

Semantically, it's "Is there an error? Yes, then return the error.
Otherwise return success" where an error in the kernel is defined as a
negative integer and success as generally zero, or sometimes a small
positive integer.

Replacing these with "min()" makes the code _less_ readable.

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

On 30.05.2023 10:45, Lu Hongfei wrote:
> It would be better to replace the traditional ternary conditional
> operator with min()
> 
No. If you say something is better you should explain the benefit.

> Signed-off-by: Lu Hongfei <luhongfei@vivo.com>
> ---
>  drivers/net/phy/phy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 drivers/net/phy/phy.c
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 0c0df38cd1ab..a8beb4ab8451
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1002,7 +1002,7 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
>  	if (!ret)
>  		return -ETIMEDOUT;
>  
> -	return ret < 0 ? ret : 0;
> +	return min(ret, 0);

ret < 0 stands for is_err(ret), therefore an arithmetic operator isn't
appropriate here.

>  }
>  
>  int phy_ethtool_ksettings_set(struct phy_device *phydev,
> @@ -1526,7 +1526,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
>  		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
>  				       MDIO_PCS_CTRL1_CLKSTOP_EN);
>  
> -	return ret < 0 ? ret : 0;
> +	return min(ret, 0);
>  }
>  EXPORT_SYMBOL(phy_init_eee);
>