[PATCH net-next 0/5] Various improvements to Microsemi PHY driver

[PATCH net-next 0/5] Various improvements to Microsemi PHY driver

[Quentin Schulz]

The Microsemi PHYs have several counters so let's make them available as PHY
statistics.

The VSC 8530/31/40/41 also need to update their EEE init sequence in order to
avoid packet losses and improve performance.

This patch series also makes some minor cosmetic changes to the driver.

Quentin Schulz (3):
  net: phy: mscc: remove unneeded parenthesis
  net: phy: mscc: shorten `x != 0` condition to `x`
  net: phy: mscc: remove unneeded temporary variable

Raju Lakkaraju (2):
  net: phy: mscc: add ethtool statistics counters
  net: phy: mscc: Add EEE init sequence

 drivers/net/phy/mscc.c | 229 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 202 insertions(+), 27 deletions(-)

base-commit: 00989856964175eafbe1435a70862c2ac66cffc0
-- 
git-series 0.9.1

[PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Quentin Schulz]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

There are a few counters available in the PHY: receive errors, false
carriers, link disconnects, media CRC errors and valids counters.

So let's expose those in the PHY driver.

Use the priv structure as the next PHY to be supported has a few
additional counters.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 128 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 2d9676d..62d6e0a 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -33,6 +33,11 @@ enum rgmii_rx_clock_delay {
 #define DISABLE_PAIR_SWAP_CORR_MASK	  0x0020
 #define DISABLE_POLARITY_CORR_MASK	  0x0010
 
+#define MSCC_PHY_ERR_RX_CNT		  19
+#define MSCC_PHY_ERR_FALSE_CARRIER_CNT	  20
+#define MSCC_PHY_ERR_LINK_DISCONNECT_CNT  21
+#define ERR_CNT_MASK			  GENMASK(7, 0)
+
 #define MSCC_PHY_EXT_PHY_CNTL_1           23
 #define MAC_IF_SELECTION_MASK             0x1800
 #define MAC_IF_SELECTION_GMII             0
@@ -64,6 +69,9 @@ enum rgmii_rx_clock_delay {
 #define MSCC_PHY_PAGE_EXTENDED_2	  0x0002 /* Extended reg - page 2 */
 
 /* Extended Page 1 Registers */
+#define MSCC_PHY_CU_MEDIA_CRC_VALID_CNT	  18
+#define VALID_CRC_CNT_CRC_MASK		  GENMASK(13, 0)
+
 #define MSCC_PHY_EXT_MODE_CNTL		  19
 #define FORCE_MDI_CROSSOVER_MASK	  0x000C
 #define FORCE_MDI_CROSSOVER_MDIX	  0x000C
@@ -74,6 +82,8 @@ enum rgmii_rx_clock_delay {
 #define DOWNSHIFT_EN			  0x0010
 #define DOWNSHIFT_CNTL_POS		  2
 
+#define MSCC_PHY_EXT_PHY_CNTL_4		  23
+
 /* Extended Page 2 Registers */
 #define MSCC_PHY_RGMII_CNTL		  20
 #define RGMII_RX_CLK_DELAY_MASK		  0x0070
@@ -119,11 +129,50 @@ enum rgmii_rx_clock_delay {
 				BIT(VSC8531_FORCE_LED_OFF) | \
 				BIT(VSC8531_FORCE_LED_ON))
 
+struct vsc85xx_hw_stat {
+	const char *string;
+	u8 reg;
+	u16 page;
+	u16 mask;
+};
+
+static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {
+	{
+		.string	= "phy_receive_errors",
+		.reg	= MSCC_PHY_ERR_RX_CNT,
+		.page	= MSCC_PHY_PAGE_STANDARD,
+		.mask	= ERR_CNT_MASK,
+	}, {
+		.string	= "phy_false_carrier",
+		.reg	= MSCC_PHY_ERR_FALSE_CARRIER_CNT,
+		.page	= MSCC_PHY_PAGE_STANDARD,
+		.mask	= ERR_CNT_MASK,
+	}, {
+		.string	= "phy_cu_media_link_disconnect",
+		.reg	= MSCC_PHY_ERR_LINK_DISCONNECT_CNT,
+		.page	= MSCC_PHY_PAGE_STANDARD,
+		.mask	= ERR_CNT_MASK,
+	}, {
+		.string	= "phy_cu_media_crc_good_count",
+		.reg	= MSCC_PHY_CU_MEDIA_CRC_VALID_CNT,
+		.page	= MSCC_PHY_PAGE_EXTENDED,
+		.mask	= VALID_CRC_CNT_CRC_MASK,
+	}, {
+		.string	= "phy_cu_media_crc_error_count",
+		.reg	= MSCC_PHY_EXT_PHY_CNTL_4,
+		.page	= MSCC_PHY_PAGE_EXTENDED,
+		.mask	= ERR_CNT_MASK,
+	},
+};
+
 struct vsc8531_private {
 	int rate_magic;
 	u16 supp_led_modes;
 	u32 leds_mode[MAX_LEDS];
 	u8 nleds;
+	struct vsc85xx_hw_stat *hw_stats;
+	u64 *stats;
+	int nstats;
 };
 
 #ifdef CONFIG_OF_MDIO
@@ -148,6 +197,66 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u16 page)
 	return rc;
 }
 
+static int vsc85xx_get_sset_count(struct phy_device *phydev)
+{
+	struct vsc8531_private *priv = phydev->priv;
+
+	if (!priv)
+		return 0;
+
+	return priv->nstats;
+}
+
+static void vsc85xx_get_strings(struct phy_device *phydev, u8 *data)
+{
+	struct vsc8531_private *priv = phydev->priv;
+	int i;
+
+	if (!priv)
+		return;
+
+	for (i = 0; i < priv->nstats; i++)
+		strlcpy(data + i * ETH_GSTRING_LEN, priv->hw_stats[i].string,
+			ETH_GSTRING_LEN);
+}
+
+static u64 vsc85xx_get_stat(struct phy_device *phydev, int i)
+{
+	struct vsc8531_private *priv = phydev->priv;
+	int val;
+	u64 ret;
+
+	vsc85xx_phy_page_set(phydev, priv->hw_stats[i].page);
+
+	val = phy_read(phydev, priv->hw_stats[i].reg);
+	if (val < 0) {
+		ret = U64_MAX;
+		goto out;
+	}
+
+	val = val & priv->hw_stats[i].mask;
+	priv->stats[i] += val;
+	ret = priv->stats[i];
+
+out:
+	vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+	return ret;
+}
+
+static void vsc85xx_get_stats(struct phy_device *phydev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	struct vsc8531_private *priv = phydev->priv;
+	int i;
+
+	if (!priv)
+		return;
+
+	for (i = 0; i < priv->nstats; i++)
+		data[i] = vsc85xx_get_stat(phydev, i);
+}
+
 static int vsc85xx_led_cntl_set(struct phy_device *phydev,
 				u8 led_num,
 				u8 mode)
@@ -673,6 +782,13 @@ static int vsc85xx_probe(struct phy_device *phydev)
 	vsc8531->rate_magic = rate_magic;
 	vsc8531->nleds = 2;
 	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
+	vsc8531->hw_stats = vsc85xx_hw_stats;
+	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
+	vsc8531->stats = devm_kzalloc(&phydev->mdio.dev,
+				      sizeof(u64) * vsc8531->nstats,
+				      GFP_KERNEL);
+	if (!vsc8531->stats)
+		return -ENOMEM;
 
 	return vsc85xx_dt_led_modes_get(phydev, default_mode);
 }
@@ -699,6 +815,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 },
 {
 	.phy_id		= PHY_ID_VSC8531,
@@ -720,6 +839,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 },
 {
 	.phy_id		= PHY_ID_VSC8540,
@@ -741,6 +863,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 },
 {
 	.phy_id		= PHY_ID_VSC8541,
@@ -762,6 +887,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 }
 
 };
-- 
git-series 0.9.1

[PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

[Quentin Schulz]

From: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>

Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
Ethernet initialization sequence.
In order to avoid certain link state errors that could result in link
drops and packet loss, the physical coding sublayer (PCS) must be
updated with settings related to EEE in order to improve performance.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 62d6e0a..c0a9ea9 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -67,6 +67,7 @@ enum rgmii_rx_clock_delay {
 #define MSCC_PHY_PAGE_STANDARD		  0x0000 /* Standard registers */
 #define MSCC_PHY_PAGE_EXTENDED		  0x0001 /* Extended registers */
 #define MSCC_PHY_PAGE_EXTENDED_2	  0x0002 /* Extended reg - page 2 */
+#define MSCC_PHY_PAGE_TR		  0x52b5 /* Token ring registers */
 
 /* Extended Page 1 Registers */
 #define MSCC_PHY_CU_MEDIA_CRC_VALID_CNT	  18
@@ -100,6 +101,13 @@ enum rgmii_rx_clock_delay {
 #define SECURE_ON_ENABLE		  0x8000
 #define SECURE_ON_PASSWD_LEN_4		  0x4000
 
+/* Token ring page Registers */
+#define MSCC_PHY_TR_CNTL		  16
+#define TR_WRITE			  0x8000
+#define TR_ADDR(x)			  (0x7fff & (x))
+#define MSCC_PHY_TR_LSB			  17
+#define MSCC_PHY_TR_MSB			  18
+
 /* Microsemi PHY ID's */
 #define PHY_ID_VSC8530			  0x00070560
 #define PHY_ID_VSC8531			  0x00070570
@@ -685,6 +693,48 @@ static int vsc85xx_set_tunable(struct phy_device *phydev,
 	}
 }
 
+static void vsc85xx_tr_write(struct phy_device *phydev, u16 addr, u32 val)
+{
+	phy_write(phydev, MSCC_PHY_TR_MSB, val >> 16);
+	phy_write(phydev, MSCC_PHY_TR_LSB, val & GENMASK(15, 0));
+	phy_write(phydev, MSCC_PHY_TR_CNTL, TR_WRITE | TR_ADDR(addr));
+}
+
+static int vsc85xx_eee_init_seq_set(struct phy_device *phydev)
+{
+	int rc;
+
+	mutex_lock(&phydev->lock);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_TR);
+	if (rc)
+		goto out_unlock;
+
+	vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
+	vsc85xx_tr_write(phydev, 0x1686, 0x00000004);
+	vsc85xx_tr_write(phydev, 0x168c, 0x00d2c46f);
+	vsc85xx_tr_write(phydev, 0x17a2, 0x00000620);
+	vsc85xx_tr_write(phydev, 0x16a0, 0x00eeffdd);
+	vsc85xx_tr_write(phydev, 0x16a6, 0x00071448);
+	vsc85xx_tr_write(phydev, 0x16a4, 0x0013132f);
+	vsc85xx_tr_write(phydev, 0x16a8, 0x00000000);
+	vsc85xx_tr_write(phydev, 0x0ffc, 0x00c0a028);
+	vsc85xx_tr_write(phydev, 0x0fe8, 0x0091b06c);
+	vsc85xx_tr_write(phydev, 0x0fea, 0x00041600);
+	vsc85xx_tr_write(phydev, 0x0f80, 0x00000af4);
+	vsc85xx_tr_write(phydev, 0x0fec, 0x00901809);
+	vsc85xx_tr_write(phydev, 0x0fee, 0x0000a6a1);
+	vsc85xx_tr_write(phydev, 0x0ffe, 0x00b01007);
+	vsc85xx_tr_write(phydev, 0x16b0, 0x00eeff00);
+	vsc85xx_tr_write(phydev, 0x16b2, 0x00007000);
+	vsc85xx_tr_write(phydev, 0x16b4, 0x00000814);
+
+out_unlock:
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
 	int rc, i;
@@ -702,6 +752,10 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	if (rc)
 		return rc;
 
+	rc = vsc85xx_eee_init_seq_set(phydev);
+	if (rc)
+		return rc;
+
 	for (i = 0; i < vsc8531->nleds; i++) {
 		rc = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
 		if (rc)
-- 
git-series 0.9.1

[PATCH net-next 3/5] net: phy: mscc: remove unneeded parenthesis

[Quentin Schulz]

The == operator precedes the || operator, so we can remove the
parenthesis around (a == b) || (c == d).

The condition is rather explicit and short so removing the parenthesis
definitely does not make it harder to read.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index c0a9ea9..734d9fb 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -301,7 +301,7 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
 	u16 reg_val;
 
 	reg_val = phy_read(phydev, MSCC_PHY_BYPASS_CONTROL);
-	if ((mdix == ETH_TP_MDI) || (mdix == ETH_TP_MDI_X)) {
+	if (mdix == ETH_TP_MDI || mdix == ETH_TP_MDI_X) {
 		reg_val |= (DISABLE_PAIR_SWAP_CORR_MASK |
 			    DISABLE_POLARITY_CORR_MASK  |
 			    DISABLE_HP_AUTO_MDIX_MASK);
-- 
git-series 0.9.1

[PATCH net-next 4/5] net: phy: mscc: shorten `x != 0` condition to `x`

[Quentin Schulz]

`if (x != 0)` is basically a more verbose version of `if (x)` so let's
use the latter so it's consistent throughout the whole driver.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 734d9fb..efa9352 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -311,11 +311,11 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
 			     DISABLE_HP_AUTO_MDIX_MASK);
 	}
 	rc = phy_write(phydev, MSCC_PHY_BYPASS_CONTROL, reg_val);
-	if (rc != 0)
+	if (rc)
 		return rc;
 
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
-	if (rc != 0)
+	if (rc)
 		return rc;
 
 	reg_val = phy_read(phydev, MSCC_PHY_EXT_MODE_CNTL);
@@ -325,11 +325,11 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
 	else if (mdix == ETH_TP_MDI_X)
 		reg_val |= FORCE_MDI_CROSSOVER_MDIX;
 	rc = phy_write(phydev, MSCC_PHY_EXT_MODE_CNTL, reg_val);
-	if (rc != 0)
+	if (rc)
 		return rc;
 
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
-	if (rc != 0)
+	if (rc)
 		return rc;
 
 	return genphy_restart_aneg(phydev);
@@ -341,7 +341,7 @@ static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
 	u16 reg_val;
 
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
-	if (rc != 0)
+	if (rc)
 		goto out;
 
 	reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
@@ -373,14 +373,14 @@ static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
 	}
 
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
-	if (rc != 0)
+	if (rc)
 		goto out;
 
 	reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
 	reg_val &= ~(DOWNSHIFT_CNTL_MASK);
 	reg_val |= count;
 	rc = phy_write(phydev, MSCC_PHY_ACTIPHY_CNTL, reg_val);
-	if (rc != 0)
+	if (rc)
 		goto out;
 
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
@@ -401,7 +401,7 @@ static int vsc85xx_wol_set(struct phy_device *phydev,
 
 	mutex_lock(&phydev->lock);
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
-	if (rc != 0)
+	if (rc)
 		goto out_unlock;
 
 	if (wol->wolopts & WAKE_MAGIC) {
@@ -439,7 +439,7 @@ static int vsc85xx_wol_set(struct phy_device *phydev,
 	phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
 
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
-	if (rc != 0)
+	if (rc)
 		goto out_unlock;
 
 	if (wol->wolopts & WAKE_MAGIC) {
@@ -447,14 +447,14 @@ static int vsc85xx_wol_set(struct phy_device *phydev,
 		reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
 		reg_val |= MII_VSC85XX_INT_MASK_WOL;
 		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
-		if (rc != 0)
+		if (rc)
 			goto out_unlock;
 	} else {
 		/* Disable the WOL interrupt */
 		reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
 		reg_val &= (~MII_VSC85XX_INT_MASK_WOL);
 		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
-		if (rc != 0)
+		if (rc)
 			goto out_unlock;
 	}
 	/* Clear WOL iterrupt status */
@@ -595,13 +595,13 @@ static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)
 
 	mutex_lock(&phydev->lock);
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
-	if (rc != 0)
+	if (rc)
 		goto out_unlock;
 	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
 	reg_val &= ~(EDGE_RATE_CNTL_MASK);
 	reg_val |= (edge_rate << EDGE_RATE_CNTL_POS);
 	rc = phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
-	if (rc != 0)
+	if (rc)
 		goto out_unlock;
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
 
@@ -636,7 +636,7 @@ static int vsc85xx_mac_if_set(struct phy_device *phydev,
 		goto out_unlock;
 	}
 	rc = phy_write(phydev, MSCC_PHY_EXT_PHY_CNTL_1, reg_val);
-	if (rc != 0)
+	if (rc)
 		goto out_unlock;
 
 	rc = genphy_soft_reset(phydev);
@@ -655,7 +655,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 	mutex_lock(&phydev->lock);
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
-	if (rc != 0)
+	if (rc)
 		goto out_unlock;
 
 	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
-- 
git-series 0.9.1

[PATCH net-next 5/5] net: phy: mscc: remove unneeded temporary variable

[Quentin Schulz]

Here, the rc variable is either used only for the condition right after
the assignment or right before being used as the return value of the
function it's being used in.

So let's remove this unneeded temporary variable whenever possible.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index efa9352..24f4754 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -199,10 +199,7 @@ static const struct vsc8531_edge_rate_table edge_table[] = {
 
 static int vsc85xx_phy_page_set(struct phy_device *phydev, u16 page)
 {
-	int rc;
-
-	rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
-	return rc;
+	return phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
 }
 
 static int vsc85xx_get_sset_count(struct phy_device *phydev)
@@ -504,7 +501,7 @@ static void vsc85xx_wol_get(struct phy_device *phydev,
 static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 {
 	u32 vdd, sd;
-	int rc, i, j;
+	int i, j;
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
 	u8 sd_array_size = ARRAY_SIZE(edge_table[0].slowdown);
@@ -512,12 +509,10 @@ static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 	if (!of_node)
 		return -ENODEV;
 
-	rc = of_property_read_u32(of_node, "vsc8531,vddmac", &vdd);
-	if (rc != 0)
+	if (of_property_read_u32(of_node, "vsc8531,vddmac", &vdd))
 		vdd = MSCC_VDDMAC_3300;
 
-	rc = of_property_read_u32(of_node, "vsc8531,edge-slowdown", &sd);
-	if (rc != 0)
+	if (of_property_read_u32(of_node, "vsc8531,edge-slowdown", &sd))
 		sd = 0;
 
 	for (i = 0; i < ARRAY_SIZE(edge_table); i++)
@@ -762,9 +757,7 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 			return rc;
 	}
 
-	rc = genphy_config_init(phydev);
-
-	return rc;
+	return genphy_config_init(phydev);
 }
 
 static int vsc85xx_ack_interrupt(struct phy_device *phydev)
-- 
git-series 0.9.1

Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Andrew Lunn]

Hi Quentin

> +static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {

You could add a const to that.

> +	{
> +		.string	= "phy_receive_errors",
> +		.reg	= MSCC_PHY_ERR_RX_CNT,
> +		.page	= MSCC_PHY_PAGE_STANDARD,
> +		.mask	= ERR_CNT_MASK,
> +	}, {
> +		.string	= "phy_false_carrier",
> +		.reg	= MSCC_PHY_ERR_FALSE_CARRIER_CNT,
> +		.page	= MSCC_PHY_PAGE_STANDARD,
> +		.mask	= ERR_CNT_MASK,
> +	}, {
> +		.string	= "phy_cu_media_link_disconnect",
> +		.reg	= MSCC_PHY_ERR_LINK_DISCONNECT_CNT,
> +		.page	= MSCC_PHY_PAGE_STANDARD,
> +		.mask	= ERR_CNT_MASK,
> +	}, {
> +		.string	= "phy_cu_media_crc_good_count",
> +		.reg	= MSCC_PHY_CU_MEDIA_CRC_VALID_CNT,
> +		.page	= MSCC_PHY_PAGE_EXTENDED,
> +		.mask	= VALID_CRC_CNT_CRC_MASK,
> +	}, {
> +		.string	= "phy_cu_media_crc_error_count",
> +		.reg	= MSCC_PHY_EXT_PHY_CNTL_4,
> +		.page	= MSCC_PHY_PAGE_EXTENDED,
> +		.mask	= ERR_CNT_MASK,
> +	},
> +};

> +static u64 vsc85xx_get_stat(struct phy_device *phydev, int i)
> +{
> +	struct vsc8531_private *priv = phydev->priv;
> +	int val;
> +	u64 ret;
> +
> +	vsc85xx_phy_page_set(phydev, priv->hw_stats[i].page);

I might of asked this before...

Does changing the page effect registers in the lower range? It is
possible for other operations to happen at the same time, and you
don't want for example a status read to happen from some other
extended page register because a statistics read is happening.

phy_read_page() and phy_write_page() will do the needed locking if
this is an issue.

> @@ -673,6 +782,13 @@ static int vsc85xx_probe(struct phy_device *phydev)
>  	vsc8531->rate_magic = rate_magic;
>  	vsc8531->nleds = 2;
>  	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
> +	vsc8531->hw_stats = vsc85xx_hw_stats;
> +	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
> +	vsc8531->stats = devm_kzalloc(&phydev->mdio.dev,
> +				      sizeof(u64) * vsc8531->nstats,
> +				      GFP_KERNEL);

devm_kmalloc_array()? The security people prefer that.

> +	if (!vsc8531->stats)
> +		return -ENOMEM;
>  
>  	return vsc85xx_dt_led_modes_get(phydev, default_mode);
>  }

   Andrew

Re: [PATCH net-next 3/5] net: phy: mscc: remove unneeded parenthesis

[Andrew Lunn]

On Fri, Sep 14, 2018 at 10:33:45AM +0200, Quentin Schulz wrote:
> The == operator precedes the || operator, so we can remove the
> parenthesis around (a == b) || (c == d).
> 
> The condition is rather explicit and short so removing the parenthesis
> definitely does not make it harder to read.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 4/5] net: phy: mscc: shorten `x != 0` condition to `x`

[Andrew Lunn]

On Fri, Sep 14, 2018 at 10:33:46AM +0200, Quentin Schulz wrote:
> `if (x != 0)` is basically a more verbose version of `if (x)` so let's
> use the latter so it's consistent throughout the whole driver.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 5/5] net: phy: mscc: remove unneeded temporary variable

[Andrew Lunn]

On Fri, Sep 14, 2018 at 10:33:47AM +0200, Quentin Schulz wrote:
> Here, the rc variable is either used only for the condition right after
> the assignment or right before being used as the return value of the
> function it's being used in.
> 
> So let's remove this unneeded temporary variable whenever possible.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]

Hi Andrew,

On Fri, Sep 14, 2018 at 03:01:56PM +0200, Andrew Lunn wrote:
> Hi Quentin
> 
> > +static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {
> 
> You could add a const to that.
> 

ACK.

> > +	{
> > +		.string	= "phy_receive_errors",
> > +		.reg	= MSCC_PHY_ERR_RX_CNT,
> > +		.page	= MSCC_PHY_PAGE_STANDARD,
> > +		.mask	= ERR_CNT_MASK,
> > +	}, {
> > +		.string	= "phy_false_carrier",
> > +		.reg	= MSCC_PHY_ERR_FALSE_CARRIER_CNT,
> > +		.page	= MSCC_PHY_PAGE_STANDARD,
> > +		.mask	= ERR_CNT_MASK,
> > +	}, {
> > +		.string	= "phy_cu_media_link_disconnect",
> > +		.reg	= MSCC_PHY_ERR_LINK_DISCONNECT_CNT,
> > +		.page	= MSCC_PHY_PAGE_STANDARD,
> > +		.mask	= ERR_CNT_MASK,
> > +	}, {
> > +		.string	= "phy_cu_media_crc_good_count",
> > +		.reg	= MSCC_PHY_CU_MEDIA_CRC_VALID_CNT,
> > +		.page	= MSCC_PHY_PAGE_EXTENDED,
> > +		.mask	= VALID_CRC_CNT_CRC_MASK,
> > +	}, {
> > +		.string	= "phy_cu_media_crc_error_count",
> > +		.reg	= MSCC_PHY_EXT_PHY_CNTL_4,
> > +		.page	= MSCC_PHY_PAGE_EXTENDED,
> > +		.mask	= ERR_CNT_MASK,
> > +	},
> > +};
> 
> > +static u64 vsc85xx_get_stat(struct phy_device *phydev, int i)
> > +{
> > +	struct vsc8531_private *priv = phydev->priv;
> > +	int val;
> > +	u64 ret;
> > +
> > +	vsc85xx_phy_page_set(phydev, priv->hw_stats[i].page);
> 
> I might of asked this before...
> 
> Does changing the page effect registers in the lower range? It is
> possible for other operations to happen at the same time, and you
> don't want for example a status read to happen from some other
> extended page register because a statistics read is happening.
> 

When you change a page, you basically can access only the registers in
this page so if there are two functions requesting different pages at
the same time or registers of different pages, it won't work well
indeed.

> phy_read_page() and phy_write_page() will do the needed locking if
> this is an issue.
> 

That's awesome! Didn't know it existed. Thanks a ton!

Well, that means I should migrate the whole driver to use
phy_read/write_paged instead of the phy_read/write that is currently in
use.

That's impacting performance though as per phy_read/write_paged we read
the current page, set the desired page, read/write the register, set the
old page back. That's 4 times more operations. Couldn't we use the
phy_device mutex instead (as it's currently done in the whole driver)?
Or is it worse/comparable in performance to the suggested solution?

> > @@ -673,6 +782,13 @@ static int vsc85xx_probe(struct phy_device *phydev)
> >  	vsc8531->rate_magic = rate_magic;
> >  	vsc8531->nleds = 2;
> >  	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
> > +	vsc8531->hw_stats = vsc85xx_hw_stats;
> > +	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
> > +	vsc8531->stats = devm_kzalloc(&phydev->mdio.dev,
> > +				      sizeof(u64) * vsc8531->nstats,
> > +				      GFP_KERNEL);
> 
> devm_kmalloc_array()? The security people prefer that.
> 

ACK.

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Andrew Lunn]

> When you change a page, you basically can access only the registers in
> this page so if there are two functions requesting different pages at
> the same time or registers of different pages, it won't work well
> indeed.
> 
> > phy_read_page() and phy_write_page() will do the needed locking if
> > this is an issue.
> > 
> 
> That's awesome! Didn't know it existed. Thanks a ton!
> 
> Well, that means I should migrate the whole driver to use
> phy_read/write_paged instead of the phy_read/write that is currently in
> use.
> 
> That's impacting performance though as per phy_read/write_paged we read
> the current page, set the desired page, read/write the register, set the
> old page back. That's 4 times more operations.

You can use the lower level locking primatives. See m88e1318_set_wol()
for example.

> Couldn't we use the
> phy_device mutex instead (as it's currently done in the whole driver)?
> Or is it worse/comparable in performance to the suggested solution?

Russell King found a race condition where this breaks. You cannot hold
the phy_device mutex everywhere.

    Andrew

Re: [PATCH net-next 5/5] net: phy: mscc: remove unneeded temporary variable

[Florian Fainelli]

On 09/14/18 01:33, Quentin Schulz wrote:
> Here, the rc variable is either used only for the condition right after
> the assignment or right before being used as the return value of the
> function it's being used in.
> 
> So let's remove this unneeded temporary variable whenever possible.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

[Florian Fainelli]

On 09/14/18 01:33, Quentin Schulz wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> 
> Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
> Ethernet initialization sequence.
> In order to avoid certain link state errors that could result in link
> drops and packet loss, the physical coding sublayer (PCS) must be
> updated with settings related to EEE in order to improve performance.
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---

[snip]

> +	vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);

Can you just make this an array of register + value pair? That would be
less error prone in case you need to update that sequence in the future.

With that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 3/5] net: phy: mscc: remove unneeded parenthesis

[Florian Fainelli]

On 9/14/2018 1:33 AM, Quentin Schulz wrote:
> The == operator precedes the || operator, so we can remove the
> parenthesis around (a == b) || (c == d).
> 
> The condition is rather explicit and short so removing the parenthesis
> definitely does not make it harder to read.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 4/5] net: phy: mscc: shorten `x != 0` condition to `x`

[Florian Fainelli]

On 9/14/2018 1:33 AM, Quentin Schulz wrote:
> `if (x != 0)` is basically a more verbose version of `if (x)` so let's
> use the latter so it's consistent throughout the whole driver.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

Hi Florian,

On Fri, Sep 14, 2018 at 07:21:09PM -0700, Florian Fainelli wrote:
> 
> 
> On 09/14/18 01:33, Quentin Schulz wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> > 
> > Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
> > Ethernet initialization sequence.
> > In order to avoid certain link state errors that could result in link
> > drops and packet loss, the physical coding sublayer (PCS) must be
> > updated with settings related to EEE in order to improve performance.
> > 
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> 
> [snip]
> 
> > +	vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
> 
> Can you just make this an array of register + value pair? That would be

Sure, I'll.

> less error prone in case you need to update that sequence in the future.
> 

I'm curious about the kind of errors you're worrying about or have
experienced. Do you have any particular example or thought in mind?

> With that:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

[Florian Fainelli]

On 10/01/2018 01:51 AM, Quentin Schulz wrote:
> Hi Florian,
> 
> On Fri, Sep 14, 2018 at 07:21:09PM -0700, Florian Fainelli wrote:
>>
>>
>> On 09/14/18 01:33, Quentin Schulz wrote:
>>> From: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
>>>
>>> Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
>>> Ethernet initialization sequence.
>>> In order to avoid certain link state errors that could result in link
>>> drops and packet loss, the physical coding sublayer (PCS) must be
>>> updated with settings related to EEE in order to improve performance.
>>>
>>> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
>>> ---
>>
>> [snip]
>>
>>> +	vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
>>
>> Can you just make this an array of register + value pair? That would be
> 
> Sure, I'll.
> 
>> less error prone in case you need to update that sequence in the future.
>>
> 
> I'm curious about the kind of errors you're worrying about or have
> experienced. Do you have any particular example or thought in mind?

Since this is just a completely non documented sequence likely given
as-is by the vendor, there could be in the future an arbitrary number of
changes made to that sequence because reasons. It seems to me that
putting that sequence in an array, instead of having to produce the
right sequence of calls, inlined in the source, is more manageable, and
will lead to an easier process if back porting/forward porting is necessary.
-- 
Florian

Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 3972 bytes --]

Hi Russel,

Adding you to the discussion as you're the author and commiter of the
patch adding support for all the paged access in the phy core.

On Fri, Sep 14, 2018 at 03:29:59PM +0200, Andrew Lunn wrote:
> > When you change a page, you basically can access only the registers in
> > this page so if there are two functions requesting different pages at
> > the same time or registers of different pages, it won't work well
> > indeed.
> > 
> > > phy_read_page() and phy_write_page() will do the needed locking if
> > > this is an issue.
> > > 
> > 
> > That's awesome! Didn't know it existed. Thanks a ton!
> > 
> > Well, that means I should migrate the whole driver to use
> > phy_read/write_paged instead of the phy_read/write that is currently in
> > use.
> > 
> > That's impacting performance though as per phy_read/write_paged we read
> > the current page, set the desired page, read/write the register, set the
> > old page back. That's 4 times more operations.
> 
> You can use the lower level locking primatives. See m88e1318_set_wol()
> for example.
> 

I'm converting the drivers/net/phy/mscc.c driver to make use of the
paged accesses but I'm hitting something confusing to me.

Firstly, just to be sure, I should use paged accesses only for read/write
outside of the standard page, right? I'm guessing that because we need
to be able to use the genphy functions which are using phy_write/read
and not __phy_write/read, thus we assume the mdio lock is not taken
(which is taken by phy_select/restore_page) and those functions
read/write to the standard page.

Secondly, I should refactor the driver to do the following:

oldpage = phy_select_page();
if (oldpage < 0) {
	phy_restore_page();
	error_path;
}

[...]
/* paged accesses */
__phy_write/read();
[...]

phy_restore_page();

I assume this is the correct way to handle paged accesses. Let me know
if it's not clear enough or wrong. (depending on the function, we could
of course put phy_restore_page in the error_path).

Now, I saw that phy_restore_page takes the phydev, the oldpage and a ret
parameters[1].

The (ret >= 0 && r < 0) condition of [2] seems counterintuitive to me.

ret being the argument passed to the function and r being the return of
__phy_write_page (which is phydev->drv->phy_write_page()).

In my understanding of C best practices, any return value equal to zero
marks a successful call to the function.

That would mean that with:
if (ret >= 0 && r < 0)
	ret = r;

If ret is greather than 0, if __phy_write_page is successful (r == 0),
ret will be > 0, which would result in phy_restore_page not returning 0
thus signaling (IMO) an error occured in phy_restore_page.

One example is the following:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage);

If phy_select_page is successful, return phy_restore_page(phydev,
oldpage, oldpage) would return the value of oldpage which can be
different from 0.

This code could (I think) be working with `if (ret >= 0 && r <= 0)` (or
even `if (ret >= 0)`).

Now to have the same behaviour, I need to do:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage > 0 ? 0 : oldpage);

Another example is:
oldpage = phy_select_page(phydev, new_page);
ret = `any function returning a value > 0 in case of success and < 0 in
case of failure`().
return phy_restore_page(phydev, oldpage, ret);

Is there any reason for not wanting to overwrite the ret value when
__phy_write_page is successful in phy_restore_page?

I'd say that it could be more readable without the ternary condition in
the argument of phy_restore_page.

Let me know your thoughts on this.

Thanks,
Quentin

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L444
[2] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L454

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 5101 bytes --]

Hi Russel,

On Tue, Oct 02, 2018 at 03:51:11PM +0200, Quentin Schulz wrote:
> Hi Russel,
> 
> Adding you to the discussion as you're the author and commiter of the
> patch adding support for all the paged access in the phy core.
> 
> On Fri, Sep 14, 2018 at 03:29:59PM +0200, Andrew Lunn wrote:
> > > When you change a page, you basically can access only the registers in
> > > this page so if there are two functions requesting different pages at
> > > the same time or registers of different pages, it won't work well
> > > indeed.
> > > 
> > > > phy_read_page() and phy_write_page() will do the needed locking if
> > > > this is an issue.
> > > > 
> > > 
> > > That's awesome! Didn't know it existed. Thanks a ton!
> > > 
> > > Well, that means I should migrate the whole driver to use
> > > phy_read/write_paged instead of the phy_read/write that is currently in
> > > use.
> > > 
> > > That's impacting performance though as per phy_read/write_paged we read
> > > the current page, set the desired page, read/write the register, set the
> > > old page back. That's 4 times more operations.
> > 
> > You can use the lower level locking primatives. See m88e1318_set_wol()
> > for example.
> > 
> 
> I'm converting the drivers/net/phy/mscc.c driver to make use of the
> paged accesses but I'm hitting something confusing to me.
> 
> Firstly, just to be sure, I should use paged accesses only for read/write
> outside of the standard page, right? I'm guessing that because we need
> to be able to use the genphy functions which are using phy_write/read
> and not __phy_write/read, thus we assume the mdio lock is not taken
> (which is taken by phy_select/restore_page) and those functions
> read/write to the standard page.
> 
> Secondly, I should refactor the driver to do the following:
> 
> oldpage = phy_select_page();
> if (oldpage < 0) {
> 	phy_restore_page();
> 	error_path;
> }
> 
> [...]
> /* paged accesses */
> __phy_write/read();
> [...]
> 
> phy_restore_page();
> 
> I assume this is the correct way to handle paged accesses. Let me know
> if it's not clear enough or wrong. (depending on the function, we could
> of course put phy_restore_page in the error_path).
> 
> Now, I saw that phy_restore_page takes the phydev, the oldpage and a ret
> parameters[1].
> 
> The (ret >= 0 && r < 0) condition of [2] seems counterintuitive to me.
> 
> ret being the argument passed to the function and r being the return of
> __phy_write_page (which is phydev->drv->phy_write_page()).
> 
> In my understanding of C best practices, any return value equal to zero
> marks a successful call to the function.
> 
> That would mean that with:
> if (ret >= 0 && r < 0)
> 	ret = r;
> 
> If ret is greather than 0, if __phy_write_page is successful (r == 0),
> ret will be > 0, which would result in phy_restore_page not returning 0
> thus signaling (IMO) an error occured in phy_restore_page.
> 
> One example is the following:
> oldpage = phy_select_page(phydev, new_page);
> [...]
> return phy_restore_page(phydev, oldpage, oldpage);
> 
> If phy_select_page is successful, return phy_restore_page(phydev,
> oldpage, oldpage) would return the value of oldpage which can be
> different from 0.
> 
> This code could (I think) be working with `if (ret >= 0 && r <= 0)` (or
> even `if (ret >= 0)`).
> 
> Now to have the same behaviour, I need to do:
> oldpage = phy_select_page(phydev, new_page);
> [...]
> return phy_restore_page(phydev, oldpage, oldpage > 0 ? 0 : oldpage);
> 
> Another example is:
> oldpage = phy_select_page(phydev, new_page);
> ret = `any function returning a value > 0 in case of success and < 0 in
> case of failure`().
> return phy_restore_page(phydev, oldpage, ret);
> 

The whole point was there. We're trying to propagate return values
through phy_restore_page and only overwrite it if it's 0. However, there
are some functions that return something different from 0 (e.g. size of
something that is handled or returned) and are still valid and wanted to
be propagated. If we were to overwrite the return value with 0 if
__phy_write_page is returning 0, we would need to use a temporary
variable to not overwrite the return value before calling
phy_restore_page.

With my suggestion, we would need to use a temporary variable to keep a
> 0 return values while calling phy_restore_page but not when we want
phy_restore_page to return 0 even when the return value before calling
phy_restore_page is > 0.

With the current behaviour, we would need to use a temporary value (or a
ternary condition as given as an example in original mail) when we want
to return 0 only when no error happens in phy_restore_page and the
return value before calling phy_restore_page was >= 0. We would not need
to use a temporary variable when phy_restore_page finishes without error
and we want to keep the return value before calling phy_restore_page if
it's >=0.

So basically, that's down to a technical choice and none is perfect.
Sorry for bothering.

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]