linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver
@ 2018-10-08 10:07 Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions Quentin Schulz
                   ` (6 more replies)
  0 siblings, 7 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-10-08 10:07 UTC (permalink / raw)
  To: davem, andrew, f.fainelli
  Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	alexandre.belloni, Quentin Schulz

The Microsemi PHYs have multiple banks of registers (called pages).
Registers can only be accessed from one page, if we need a register from
another page, we need to switch the page and the registers of all other
pages are not accessible anymore.

Basically, to read register 5 from page 0, 1, 2, etc., you do the same
phy_read(phydev, 5); but you need to set the desired page beforehand.

In order to guarantee that two concurrent functions do not change the
page, we need to do some locking per page. This can be achieved with the
use of phy_select_page and phy_restore_page functions but phy_write/read
calls in-between those two functions shall be replaced by their
lock-free alternative __phy_write/read.

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.

Thanks,
Quentin

v3:
  - add reviewed-by,
  - use phy_read/write/modify_paged whenever possible instead of the
  combo phy_select_page, __phy_read/write/modify, phy_restore_page when
  only one __phy_read/write/modify was executed,

v2:
  - add patch to migrate MSCC driver to use phy_restore/select_page,
  - migrate all patches from v1 to use those two functions,
  - put the multiple lines of constants writes in an array and iterate over
  it to write the values,
  - add reviewed-bys,

Quentin Schulz (4):
  net: phy: mscc: migrate to phy_select/restore_page functions
  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 | 352 +++++++++++++++++++++++++++++------------
 1 file changed, 255 insertions(+), 97 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
@ 2018-10-08 10:07 ` Quentin Schulz
  2018-11-19 14:57   ` Andreas Schwab
  2018-10-08 10:07 ` [PATCH net-next v3 2/6] net: phy: mscc: add ethtool statistics counters Quentin Schulz
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Quentin Schulz @ 2018-10-08 10:07 UTC (permalink / raw)
  To: davem, andrew, f.fainelli
  Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	alexandre.belloni, Quentin Schulz

The Microsemi PHYs have multiple banks of registers (called pages).
Registers can only be accessed from one page, if we need a register from
another page, we need to switch the page and the registers of all other
pages are not accessible anymore.

Basically, to read register 5 from page 0, 1, 2, etc., you do the same
phy_read(phydev, 5); but you need to set the desired page beforehand.

In order to guarantee that two concurrent functions do not change the
page, we need to do some locking per page. This can be achieved with the
use of phy_select_page and phy_restore_page functions but phy_write/read
calls in-between those two functions shall be replaced by their
lock-free alternative __phy_write/read.

Let's migrate this driver to those functions.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 146 +++++++++++++++++------------------------
 1 file changed, 62 insertions(+), 84 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 7d0384e26c99..52198be46c68 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -140,12 +140,14 @@ static const struct vsc8531_edge_rate_table edge_table[] = {
 };
 #endif /* CONFIG_OF_MDIO */
 
-static int vsc85xx_phy_page_set(struct phy_device *phydev, u16 page)
+static int vsc85xx_phy_read_page(struct phy_device *phydev)
 {
-	int rc;
+	return __phy_read(phydev, MSCC_EXT_PAGE_ACCESS);
+}
 
-	rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
-	return rc;
+static int vsc85xx_phy_write_page(struct phy_device *phydev, int page)
+{
+	return __phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
 }
 
 static int vsc85xx_led_cntl_set(struct phy_device *phydev,
@@ -197,22 +199,17 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
 	if (rc != 0)
 		return rc;
 
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
-	if (rc != 0)
-		return rc;
+	reg_val = 0;
 
-	reg_val = phy_read(phydev, MSCC_PHY_EXT_MODE_CNTL);
-	reg_val &= ~(FORCE_MDI_CROSSOVER_MASK);
 	if (mdix == ETH_TP_MDI)
-		reg_val |= FORCE_MDI_CROSSOVER_MDI;
+		reg_val = FORCE_MDI_CROSSOVER_MDI;
 	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)
-		return rc;
+		reg_val = FORCE_MDI_CROSSOVER_MDIX;
 
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
-	if (rc != 0)
+	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED,
+			      MSCC_PHY_EXT_MODE_CNTL, FORCE_MDI_CROSSOVER_MASK,
+			      reg_val);
+	if (rc < 0)
 		return rc;
 
 	return genphy_restart_aneg(phydev);
@@ -220,30 +217,24 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
 
 static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
 {
-	int rc;
 	u16 reg_val;
 
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
-	if (rc != 0)
-		goto out;
+	reg_val = phy_read_paged(phydev, MSCC_PHY_PAGE_EXTENDED,
+				 MSCC_PHY_ACTIPHY_CNTL);
+	if (reg_val < 0)
+		return reg_val;
 
-	reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
 	reg_val &= DOWNSHIFT_CNTL_MASK;
 	if (!(reg_val & DOWNSHIFT_EN))
 		*count = DOWNSHIFT_DEV_DISABLE;
 	else
 		*count = ((reg_val & ~DOWNSHIFT_EN) >> DOWNSHIFT_CNTL_POS) + 2;
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
 
-out:
-	return rc;
+	return 0;
 }
 
 static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
 {
-	int rc;
-	u16 reg_val;
-
 	if (count == DOWNSHIFT_DEV_DEFAULT_COUNT) {
 		/* Default downshift count 3 (i.e. Bit3:2 = 0b01) */
 		count = ((1 << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
@@ -255,21 +246,9 @@ static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
 		count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
 	}
 
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
-	if (rc != 0)
-		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)
-		goto out;
-
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
-
-out:
-	return rc;
+	return phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED,
+				MSCC_PHY_ACTIPHY_CNTL, DOWNSHIFT_CNTL_MASK,
+				count);
 }
 
 static int vsc85xx_wol_set(struct phy_device *phydev,
@@ -283,46 +262,48 @@ static int vsc85xx_wol_set(struct phy_device *phydev,
 	u8 *mac_addr = phydev->attached_dev->dev_addr;
 
 	mutex_lock(&phydev->lock);
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
-	if (rc != 0)
+	rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc < 0) {
+		rc = phy_restore_page(phydev, rc, rc);
 		goto out_unlock;
+	}
 
 	if (wol->wolopts & WAKE_MAGIC) {
 		/* Store the device address for the magic packet */
 		for (i = 0; i < ARRAY_SIZE(pwd); i++)
 			pwd[i] = mac_addr[5 - (i * 2 + 1)] << 8 |
 				 mac_addr[5 - i * 2];
-		phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, pwd[0]);
-		phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, pwd[1]);
-		phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, pwd[2]);
+		__phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, pwd[0]);
+		__phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, pwd[1]);
+		__phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, pwd[2]);
 	} else {
-		phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
-		phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
-		phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
+		__phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
+		__phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
+		__phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
 	}
 
 	if (wol_conf->wolopts & WAKE_MAGICSECURE) {
 		for (i = 0; i < ARRAY_SIZE(pwd); i++)
 			pwd[i] = wol_conf->sopass[5 - (i * 2 + 1)] << 8 |
 				 wol_conf->sopass[5 - i * 2];
-		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, pwd[0]);
-		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, pwd[1]);
-		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, pwd[2]);
+		__phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, pwd[0]);
+		__phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, pwd[1]);
+		__phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, pwd[2]);
 	} else {
-		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
-		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
-		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
+		__phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
+		__phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
+		__phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
 	}
 
-	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+	reg_val = __phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
 	if (wol_conf->wolopts & WAKE_MAGICSECURE)
 		reg_val |= SECURE_ON_ENABLE;
 	else
 		reg_val &= ~SECURE_ON_ENABLE;
-	phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
+	__phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
 
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
-	if (rc != 0)
+	rc = phy_restore_page(phydev, rc, rc > 0 ? 0 : rc);
+	if (rc < 0)
 		goto out_unlock;
 
 	if (wol->wolopts & WAKE_MAGIC) {
@@ -359,17 +340,17 @@ static void vsc85xx_wol_get(struct phy_device *phydev,
 	struct ethtool_wolinfo *wol_conf = wol;
 
 	mutex_lock(&phydev->lock);
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
-	if (rc != 0)
+	rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc < 0)
 		goto out_unlock;
 
-	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+	reg_val = __phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
 	if (reg_val & SECURE_ON_ENABLE)
 		wol_conf->wolopts |= WAKE_MAGICSECURE;
 	if (wol_conf->wolopts & WAKE_MAGICSECURE) {
-		pwd[0] = phy_read(phydev, MSCC_PHY_WOL_LOWER_PASSWD);
-		pwd[1] = phy_read(phydev, MSCC_PHY_WOL_MID_PASSWD);
-		pwd[2] = phy_read(phydev, MSCC_PHY_WOL_UPPER_PASSWD);
+		pwd[0] = __phy_read(phydev, MSCC_PHY_WOL_LOWER_PASSWD);
+		pwd[1] = __phy_read(phydev, MSCC_PHY_WOL_MID_PASSWD);
+		pwd[2] = __phy_read(phydev, MSCC_PHY_WOL_UPPER_PASSWD);
 		for (i = 0; i < ARRAY_SIZE(pwd); i++) {
 			wol_conf->sopass[5 - i * 2] = pwd[i] & 0x00ff;
 			wol_conf->sopass[5 - (i * 2 + 1)] = (pwd[i] & 0xff00)
@@ -377,9 +358,8 @@ static void vsc85xx_wol_get(struct phy_device *phydev,
 		}
 	}
 
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
-
 out_unlock:
+	phy_restore_page(phydev, rc, rc > 0 ? 0 : rc);
 	mutex_unlock(&phydev->lock);
 }
 
@@ -474,21 +454,11 @@ static int vsc85xx_dt_led_modes_get(struct phy_device *phydev,
 static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)
 {
 	int rc;
-	u16 reg_val;
 
 	mutex_lock(&phydev->lock);
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
-	if (rc != 0)
-		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)
-		goto out_unlock;
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
-
-out_unlock:
+	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
+			      MSCC_PHY_WOL_MAC_CONTROL, EDGE_RATE_CNTL_MASK,
+			      edge_rate << EDGE_RATE_CNTL_POS);
 	mutex_unlock(&phydev->lock);
 
 	return rc;
@@ -537,17 +507,17 @@ 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)
+	rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc < 0)
 		goto out_unlock;
 
 	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
 	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
 	reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
 	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
-	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
 
 out_unlock:
+	rc = phy_restore_page(phydev, rc, rc > 0 ? 0 : rc);
 	mutex_unlock(&phydev->lock);
 
 	return rc;
@@ -699,6 +669,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.read_page	= &vsc85xx_phy_read_page,
+	.write_page	= &vsc85xx_phy_write_page,
 },
 {
 	.phy_id		= PHY_ID_VSC8531,
@@ -720,6 +692,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.read_page	= &vsc85xx_phy_read_page,
+	.write_page	= &vsc85xx_phy_write_page,
 },
 {
 	.phy_id		= PHY_ID_VSC8540,
@@ -741,6 +715,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.read_page	= &vsc85xx_phy_read_page,
+	.write_page	= &vsc85xx_phy_write_page,
 },
 {
 	.phy_id		= PHY_ID_VSC8541,
@@ -762,6 +738,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
 	.set_tunable	= &vsc85xx_set_tunable,
+	.read_page	= &vsc85xx_phy_read_page,
+	.write_page	= &vsc85xx_phy_write_page,
 }
 
 };
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH net-next v3 2/6] net: phy: mscc: add ethtool statistics counters
  2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions Quentin Schulz
@ 2018-10-08 10:07 ` Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 3/6] net: phy: mscc: Add EEE init sequence Quentin Schulz
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-10-08 10:07 UTC (permalink / raw)
  To: davem, andrew, f.fainelli
  Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	alexandre.belloni, Raju Lakkaraju, 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.

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

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 52198be46c68..47fbab489287 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 const 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;
+	const struct vsc85xx_hw_stat *hw_stats;
+	u64 *stats;
+	int nstats;
 };
 
 #ifdef CONFIG_OF_MDIO
@@ -150,6 +199,58 @@ static int vsc85xx_phy_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
 }
 
+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;
+
+	val = phy_read_paged(phydev, priv->hw_stats[i].page,
+			     priv->hw_stats[i].reg);
+	if (val < 0)
+		return U64_MAX;
+
+	val = val & priv->hw_stats[i].mask;
+	priv->stats[i] += val;
+
+	return priv->stats[i];
+}
+
+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)
@@ -643,6 +744,12 @@ 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_kmalloc_array(&phydev->mdio.dev, vsc8531->nstats,
+					    sizeof(u64), GFP_KERNEL);
+	if (!vsc8531->stats)
+		return -ENOMEM;
 
 	return vsc85xx_dt_led_modes_get(phydev, default_mode);
 }
@@ -671,6 +778,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.set_tunable	= &vsc85xx_set_tunable,
 	.read_page	= &vsc85xx_phy_read_page,
 	.write_page	= &vsc85xx_phy_write_page,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 },
 {
 	.phy_id		= PHY_ID_VSC8531,
@@ -694,6 +804,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.set_tunable	= &vsc85xx_set_tunable,
 	.read_page	= &vsc85xx_phy_read_page,
 	.write_page	= &vsc85xx_phy_write_page,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 },
 {
 	.phy_id		= PHY_ID_VSC8540,
@@ -717,6 +830,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.set_tunable	= &vsc85xx_set_tunable,
 	.read_page	= &vsc85xx_phy_read_page,
 	.write_page	= &vsc85xx_phy_write_page,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 },
 {
 	.phy_id		= PHY_ID_VSC8541,
@@ -740,6 +856,9 @@ static struct phy_driver vsc85xx_driver[] = {
 	.set_tunable	= &vsc85xx_set_tunable,
 	.read_page	= &vsc85xx_phy_read_page,
 	.write_page	= &vsc85xx_phy_write_page,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings    = &vsc85xx_get_strings,
+	.get_stats      = &vsc85xx_get_stats,
 }
 
 };
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH net-next v3 3/6] net: phy: mscc: Add EEE init sequence
  2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 2/6] net: phy: mscc: add ethtool statistics counters Quentin Schulz
@ 2018-10-08 10:07 ` Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 4/6] net: phy: mscc: remove unneeded parenthesis Quentin Schulz
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-10-08 10:07 UTC (permalink / raw)
  To: davem, andrew, f.fainelli
  Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	alexandre.belloni, Raju Lakkaraju, 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.

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

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 47fbab489287..d304fb4df23c 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
@@ -129,6 +137,11 @@ enum rgmii_rx_clock_delay {
 				BIT(VSC8531_FORCE_LED_OFF) | \
 				BIT(VSC8531_FORCE_LED_ON))
 
+struct reg_val {
+	u16	reg;
+	u32	val;
+};
+
 struct vsc85xx_hw_stat {
 	const char *string;
 	u8 reg;
@@ -647,6 +660,54 @@ static int vsc85xx_set_tunable(struct phy_device *phydev,
 	}
 }
 
+/* mdiobus lock should be locked when using this function */
+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)
+{
+	const struct reg_val init_eee[] = {
+		{0x0f82, 0x0012b00a},
+		{0x1686, 0x00000004},
+		{0x168c, 0x00d2c46f},
+		{0x17a2, 0x00000620},
+		{0x16a0, 0x00eeffdd},
+		{0x16a6, 0x00071448},
+		{0x16a4, 0x0013132f},
+		{0x16a8, 0x00000000},
+		{0x0ffc, 0x00c0a028},
+		{0x0fe8, 0x0091b06c},
+		{0x0fea, 0x00041600},
+		{0x0f80, 0x00000af4},
+		{0x0fec, 0x00901809},
+		{0x0fee, 0x0000a6a1},
+		{0x0ffe, 0x00b01007},
+		{0x16b0, 0x00eeff00},
+		{0x16b2, 0x00007000},
+		{0x16b4, 0x00000814},
+	};
+	unsigned int i;
+	int oldpage;
+
+	mutex_lock(&phydev->lock);
+	oldpage = phy_select_page(phydev, MSCC_PHY_PAGE_TR);
+	if (oldpage < 0)
+		goto out_unlock;
+
+	for (i = 0; i < ARRAY_SIZE(init_eee); i++)
+		vsc85xx_tr_write(phydev, init_eee[i].reg, init_eee[i].val);
+
+out_unlock:
+	oldpage = phy_restore_page(phydev, oldpage, oldpage);
+	mutex_unlock(&phydev->lock);
+
+	return oldpage;
+}
+
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
 	int rc, i;
@@ -664,6 +725,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)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH net-next v3 4/6] net: phy: mscc: remove unneeded parenthesis
  2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
                   ` (2 preceding siblings ...)
  2018-10-08 10:07 ` [PATCH net-next v3 3/6] net: phy: mscc: Add EEE init sequence Quentin Schulz
@ 2018-10-08 10:07 ` Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 5/6] net: phy: mscc: shorten `x != 0` condition to `x` Quentin Schulz
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-10-08 10:07 UTC (permalink / raw)
  To: davem, andrew, f.fainelli
  Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	alexandre.belloni, 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.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
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 d304fb4df23c..0ff7803ed16b 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -300,7 +300,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);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH net-next v3 5/6] net: phy: mscc: shorten `x != 0` condition to `x`
  2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
                   ` (3 preceding siblings ...)
  2018-10-08 10:07 ` [PATCH net-next v3 4/6] net: phy: mscc: remove unneeded parenthesis Quentin Schulz
@ 2018-10-08 10:07 ` Quentin Schulz
  2018-10-08 10:07 ` [PATCH net-next v3 6/6] net: phy: mscc: remove unneeded temporary variable Quentin Schulz
  2018-10-08 17:29 ` [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver David Miller
  6 siblings, 0 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-10-08 10:07 UTC (permalink / raw)
  To: davem, andrew, f.fainelli
  Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	alexandre.belloni, 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.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 0ff7803ed16b..6bfdc168c62b 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -310,7 +310,7 @@ 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;
 
 	reg_val = 0;
@@ -425,14 +425,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 */
@@ -603,7 +603,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);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH net-next v3 6/6] net: phy: mscc: remove unneeded temporary variable
  2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
                   ` (4 preceding siblings ...)
  2018-10-08 10:07 ` [PATCH net-next v3 5/6] net: phy: mscc: shorten `x != 0` condition to `x` Quentin Schulz
@ 2018-10-08 10:07 ` Quentin Schulz
  2018-10-08 17:29 ` [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver David Miller
  6 siblings, 0 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-10-08 10:07 UTC (permalink / raw)
  To: davem, andrew, f.fainelli
  Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	alexandre.belloni, 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.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 6bfdc168c62b..7ae3e644a18f 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -481,7 +481,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);
@@ -489,12 +489,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++)
@@ -735,9 +733,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)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver
  2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
                   ` (5 preceding siblings ...)
  2018-10-08 10:07 ` [PATCH net-next v3 6/6] net: phy: mscc: remove unneeded temporary variable Quentin Schulz
@ 2018-10-08 17:29 ` David Miller
  6 siblings, 0 replies; 25+ messages in thread
From: David Miller @ 2018-10-08 17:29 UTC (permalink / raw)
  To: quentin.schulz
  Cc: andrew, f.fainelli, allan.nielsen, linux-kernel, netdev,
	thomas.petazzoni, alexandre.belloni

From: Quentin Schulz <quentin.schulz@bootlin.com>
Date: Mon,  8 Oct 2018 12:07:22 +0200

> The Microsemi PHYs have multiple banks of registers (called pages).
> Registers can only be accessed from one page, if we need a register from
> another page, we need to switch the page and the registers of all other
> pages are not accessible anymore.
> 
> Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> phy_read(phydev, 5); but you need to set the desired page beforehand.
> 
> In order to guarantee that two concurrent functions do not change the
> page, we need to do some locking per page. This can be achieved with the
> use of phy_select_page and phy_restore_page functions but phy_write/read
> calls in-between those two functions shall be replaced by their
> lock-free alternative __phy_write/read.
> 
> 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.

Series applied, thank you.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-10-08 10:07 ` [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions Quentin Schulz
@ 2018-11-19 14:57   ` Andreas Schwab
  2018-11-19 15:10     ` Andrew Lunn
  2018-11-20 13:48     ` [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config Andreas Schwab
  0 siblings, 2 replies; 25+ messages in thread
From: Andreas Schwab @ 2018-11-19 14:57 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: davem, andrew, f.fainelli, allan.nielsen, linux-kernel, netdev,
	thomas.petazzoni, alexandre.belloni, linux-riscv

On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:

> The Microsemi PHYs have multiple banks of registers (called pages).
> Registers can only be accessed from one page, if we need a register from
> another page, we need to switch the page and the registers of all other
> pages are not accessible anymore.
>
> Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> phy_read(phydev, 5); but you need to set the desired page beforehand.
>
> In order to guarantee that two concurrent functions do not change the
> page, we need to do some locking per page. This can be achieved with the
> use of phy_select_page and phy_restore_page functions but phy_write/read
> calls in-between those two functions shall be replaced by their
> lock-free alternative __phy_write/read.
>
> Let's migrate this driver to those functions.

This has some serious locking problem.

[<0>] __schedule+0x25e/0x74c
[<0>] schedule+0x1a/0x58
[<0>] schedule_preempt_disabled+0xc/0x14
[<0>] __mutex_lock.isra.0+0x10e/0x22e
[<0>] __mutex_lock_slowpath+0xe/0x16
[<0>] mutex_lock+0x22/0x2a
[<0>] mdiobus_read+0x36/0x60
[<0>] vsc85xx_config_init+0x4c/0x1e2
[<0>] phy_init_hw+0x3c/0x52
[<0>] phy_attach_direct+0xec/0x1dc
[<0>] phy_connect_direct+0x1a/0x56
[<0>] macb_probe+0x800/0xb5e [macb]
[<0>] platform_drv_probe+0x3e/0x7e
[<0>] really_probe+0xba/0x310
[<0>] driver_probe_device+0x54/0xf2
[<0>] __driver_attach+0xde/0x106
[<0>] bus_for_each_dev+0x4a/0x72
[<0>] driver_attach+0x1a/0x22
[<0>] bus_add_driver+0x1ce/0x212
[<0>] driver_register+0x3a/0xd0
[<0>] __platform_driver_register+0x3a/0x42
[<0>] macb_driver_init+0x20/0x28 [macb]
[<0>] do_one_initcall+0x48/0x128
[<0>] do_init_module+0x4a/0x186
[<0>] load_module+0xd6a/0xe6a
[<0>] sys_finit_module+0xc6/0xfc
[<0>] check_syscall_nr+0x22/0x22

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 14:57   ` Andreas Schwab
@ 2018-11-19 15:10     ` Andrew Lunn
  2018-11-19 15:13       ` Andreas Schwab
  2018-11-20 13:48     ` [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config Andreas Schwab
  1 sibling, 1 reply; 25+ messages in thread
From: Andrew Lunn @ 2018-11-19 15:10 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Quentin Schulz, davem, f.fainelli, allan.nielsen, linux-kernel,
	netdev, thomas.petazzoni, alexandre.belloni, linux-riscv

On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> 
> > The Microsemi PHYs have multiple banks of registers (called pages).
> > Registers can only be accessed from one page, if we need a register from
> > another page, we need to switch the page and the registers of all other
> > pages are not accessible anymore.
> >
> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> > phy_read(phydev, 5); but you need to set the desired page beforehand.
> >
> > In order to guarantee that two concurrent functions do not change the
> > page, we need to do some locking per page. This can be achieved with the
> > use of phy_select_page and phy_restore_page functions but phy_write/read
> > calls in-between those two functions shall be replaced by their
> > lock-free alternative __phy_write/read.
> >
> > Let's migrate this driver to those functions.
> 
> This has some serious locking problem.

Hi Andreas

Could you be more specific. Are you getting a deadlock? A WARN_ON?

Thanks,
	Andrew

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 15:10     ` Andrew Lunn
@ 2018-11-19 15:13       ` Andreas Schwab
  2018-11-19 15:28         ` Andrew Lunn
  0 siblings, 1 reply; 25+ messages in thread
From: Andreas Schwab @ 2018-11-19 15:13 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Quentin Schulz, davem, f.fainelli, allan.nielsen, linux-kernel,
	netdev, thomas.petazzoni, alexandre.belloni, linux-riscv

On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
>> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
>> 
>> > The Microsemi PHYs have multiple banks of registers (called pages).
>> > Registers can only be accessed from one page, if we need a register from
>> > another page, we need to switch the page and the registers of all other
>> > pages are not accessible anymore.
>> >
>> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
>> > phy_read(phydev, 5); but you need to set the desired page beforehand.
>> >
>> > In order to guarantee that two concurrent functions do not change the
>> > page, we need to do some locking per page. This can be achieved with the
>> > use of phy_select_page and phy_restore_page functions but phy_write/read
>> > calls in-between those two functions shall be replaced by their
>> > lock-free alternative __phy_write/read.
>> >
>> > Let's migrate this driver to those functions.
>> 
>> This has some serious locking problem.
>
> Hi Andreas
>
> Could you be more specific. Are you getting a deadlock? A WARN_ON?

See the stack trace.  That's where it hangs.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 15:13       ` Andreas Schwab
@ 2018-11-19 15:28         ` Andrew Lunn
  2018-11-19 15:40           ` Alexandre Belloni
  0 siblings, 1 reply; 25+ messages in thread
From: Andrew Lunn @ 2018-11-19 15:28 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Quentin Schulz, davem, f.fainelli, allan.nielsen, linux-kernel,
	netdev, thomas.petazzoni, alexandre.belloni, linux-riscv

On Mon, Nov 19, 2018 at 04:13:10PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
> >> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> >> 
> >> > The Microsemi PHYs have multiple banks of registers (called pages).
> >> > Registers can only be accessed from one page, if we need a register from
> >> > another page, we need to switch the page and the registers of all other
> >> > pages are not accessible anymore.
> >> >
> >> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> >> > phy_read(phydev, 5); but you need to set the desired page beforehand.
> >> >
> >> > In order to guarantee that two concurrent functions do not change the
> >> > page, we need to do some locking per page. This can be achieved with the
> >> > use of phy_select_page and phy_restore_page functions but phy_write/read
> >> > calls in-between those two functions shall be replaced by their
> >> > lock-free alternative __phy_write/read.
> >> >
> >> > Let's migrate this driver to those functions.
> >> 
> >> This has some serious locking problem.
> >
> > Hi Andreas
> >
> > Could you be more specific. Are you getting a deadlock? A WARN_ON?
> 
> See the stack trace.  That's where it hangs.

So you never said it hangs. The stacktrace helps, but a description of
what actually happens also helps. And i expect Quentin has booted this
code lots of times and not had a hang. So some hits how to reproduce
it would also help. Maybe your kernel config?

I'm interested because he is using the core mdio locking
primitives. If those are broken, i want to know.

Thanks
	Andrew

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 15:28         ` Andrew Lunn
@ 2018-11-19 15:40           ` Alexandre Belloni
  2018-11-19 15:50             ` Andreas Schwab
  0 siblings, 1 reply; 25+ messages in thread
From: Alexandre Belloni @ 2018-11-19 15:40 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Andreas Schwab, Quentin Schulz, davem, f.fainelli, allan.nielsen,
	linux-kernel, netdev, thomas.petazzoni, linux-riscv

On 19/11/2018 16:28:30+0100, Andrew Lunn wrote:
> On Mon, Nov 19, 2018 at 04:13:10PM +0100, Andreas Schwab wrote:
> > On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > > On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
> > >> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> > >> 
> > >> > The Microsemi PHYs have multiple banks of registers (called pages).
> > >> > Registers can only be accessed from one page, if we need a register from
> > >> > another page, we need to switch the page and the registers of all other
> > >> > pages are not accessible anymore.
> > >> >
> > >> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> > >> > phy_read(phydev, 5); but you need to set the desired page beforehand.
> > >> >
> > >> > In order to guarantee that two concurrent functions do not change the
> > >> > page, we need to do some locking per page. This can be achieved with the
> > >> > use of phy_select_page and phy_restore_page functions but phy_write/read
> > >> > calls in-between those two functions shall be replaced by their
> > >> > lock-free alternative __phy_write/read.
> > >> >
> > >> > Let's migrate this driver to those functions.
> > >> 
> > >> This has some serious locking problem.
> > >
> > > Hi Andreas
> > >
> > > Could you be more specific. Are you getting a deadlock? A WARN_ON?
> > 
> > See the stack trace.  That's where it hangs.
> 
> So you never said it hangs. The stacktrace helps, but a description of
> what actually happens also helps. And i expect Quentin has booted this
> code lots of times and not had a hang. So some hits how to reproduce
> it would also help. Maybe your kernel config?
> 
> I'm interested because he is using the core mdio locking
> primitives. If those are broken, i want to know.
> 

My first intuition is that he mac driver quentin is using does
phy_connect when the interface is opened while macb is doing it at probe
time. I didn't investigate but maybe this can help :)

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 15:40           ` Alexandre Belloni
@ 2018-11-19 15:50             ` Andreas Schwab
  2018-11-19 16:12               ` Andrew Lunn
  0 siblings, 1 reply; 25+ messages in thread
From: Andreas Schwab @ 2018-11-19 15:50 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Andrew Lunn, Quentin Schulz, davem, f.fainelli, allan.nielsen,
	linux-kernel, netdev, thomas.petazzoni, linux-riscv

On Nov 19 2018, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> My first intuition is that he mac driver quentin is using does
> phy_connect when the interface is opened while macb is doing it at probe
> time. I didn't investigate but maybe this can help :)

The phy probing of macb is very unreliable, perhaps it needs to be
rewritten.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 15:50             ` Andreas Schwab
@ 2018-11-19 16:12               ` Andrew Lunn
  2018-11-19 16:14                 ` Andreas Schwab
  2018-11-20 11:39                 ` Andreas Schwab
  0 siblings, 2 replies; 25+ messages in thread
From: Andrew Lunn @ 2018-11-19 16:12 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
	allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	linux-riscv

On Mon, Nov 19, 2018 at 04:50:14PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> 
> > My first intuition is that he mac driver quentin is using does
> > phy_connect when the interface is opened while macb is doing it at probe
> > time. I didn't investigate but maybe this can help :)
> 
> The phy probing of macb is very unreliable, perhaps it needs to be
> rewritten.

Hi Andreas

I still don't see why that would cause a hang.

Could you turn on lockdep and see if it reports a deadlock.

Thanks
	Andrew

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 16:12               ` Andrew Lunn
@ 2018-11-19 16:14                 ` Andreas Schwab
  2018-11-19 16:25                   ` Andrew Lunn
  2018-11-20 11:39                 ` Andreas Schwab
  1 sibling, 1 reply; 25+ messages in thread
From: Andreas Schwab @ 2018-11-19 16:14 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
	allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	linux-riscv

On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:

> Could you turn on lockdep and see if it reports a deadlock.

How do I "turn on lockdep"?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 16:14                 ` Andreas Schwab
@ 2018-11-19 16:25                   ` Andrew Lunn
  2018-11-19 16:32                     ` Andreas Schwab
  0 siblings, 1 reply; 25+ messages in thread
From: Andrew Lunn @ 2018-11-19 16:25 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
	allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	linux-riscv

On Mon, Nov 19, 2018 at 05:14:38PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > Could you turn on lockdep and see if it reports a deadlock.
> 
> How do I "turn on lockdep"?

make menuconfig
Kernel hacking
Lock Debugging (spinlocks, mutexes, etc...) 
Lock debugging: prove locking correctness

It has changes its name at some point. it used to be called
CONFIG_LOCKDEP, for locking dependencies. Anybody who has been kernel
hacking for a while still calls it lockdep.

	Andrew

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 16:25                   ` Andrew Lunn
@ 2018-11-19 16:32                     ` Andreas Schwab
  2018-11-19 16:44                       ` Andrew Lunn
  0 siblings, 1 reply; 25+ messages in thread
From: Andreas Schwab @ 2018-11-19 16:32 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
	allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	linux-riscv

On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:

> Lock debugging: prove locking correctness

404

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 16:32                     ` Andreas Schwab
@ 2018-11-19 16:44                       ` Andrew Lunn
  0 siblings, 0 replies; 25+ messages in thread
From: Andrew Lunn @ 2018-11-19 16:44 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
	allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	linux-riscv

On Mon, Nov 19, 2018 at 05:32:14PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > Lock debugging: prove locking correctness
> 

Lets see how much this gets messed up by email...

.config - Linux/arm 4.20.0-rc2 Kernel Configuration
 > Kernel hacking > Lock Debugging (spinlocks, mutexes, etc...) ──────
  ┌───────── Lock Debugging (spinlocks, mutexes, etc...) ──────────┐
  │  Arrow keys navigate the menu.  <Enter> selects submenus --->  │  
  │  (or empty submenus ----).  Highlighted letters are hotkeys.   │  
  │  Pressing <Y> includes, <N> excludes, <M> modularizes          │  
  │  features.  Press <Esc><Esc> to exit, <?> for Help, </> for    │  
  │ ┌────────────────────────────────────────────────────────────┐ │  
  │ │    [*] Lock debugging: prove locking correctness           │ │  
  │ │    [ ] Lock usage statistics                               │ │  
  │ │    -*- RT Mutex debugging, deadlock detection              │ │  
  │ │    -*- Spinlock and rw-lock debugging: basic checks        │ │  
  │ │    -*- Mutex debugging: basic checks                       │ │  
  │ │    -*- Wait/wound mutex debugging: Slowpath testing        │ │  
  │ │    -*- RW Semaphore debugging: basic checks                │ │  
  │ │    -*- Lock debugging: detect incorrect freeing of live loc│ │  
  │ │    [ ] Lock dependency engine debugging                    │ │  
  │ │    [*] Sleep inside atomic section checking                │ │  
  │ │    [ ] Locking API boot-time self-tests                    │ │  
  │ │    < > torture tests for locking                           │ │  
  │ │    < > Wait/wound mutex selftests                          │ │  
  │ └────────────────────────────────────────────────────────────┘ │  
  ├────────────────────────────────────────────────────────────────┤  
  │    <Select>    < Exit >    < Help >    < Save >    < Load >    │  
  └────────────────────────────────────────────────────────────────┘  
    
If you cannot see it at all, it probably means you are missing a
dependency:

  │   Depends on: DEBUG_KERNEL [=y] && \                           │  
  │ LOCK_DEBUGGING_SUPPORT [=y]                                    │  

    Andrew



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-19 16:12               ` Andrew Lunn
  2018-11-19 16:14                 ` Andreas Schwab
@ 2018-11-20 11:39                 ` Andreas Schwab
  2018-11-20 13:20                   ` Quentin Schulz
  1 sibling, 1 reply; 25+ messages in thread
From: Andreas Schwab @ 2018-11-20 11:39 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
	allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
	linux-riscv

On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:

> I still don't see why that would cause a hang.

[  996.370000] [<ffffffe0011d7324>] mutex_lock+0x22/0x2a
[  996.380000] [<ffffffe001039ba0>] mdiobus_read+0x36/0x60
[  996.380000] [<ffffffe00103b82c>] vsc85xx_config_init+0x4c/0x1e2

vsc85xx_config_init is calling mdiobus_read while holding the mdio_lock.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
  2018-11-20 11:39                 ` Andreas Schwab
@ 2018-11-20 13:20                   ` Quentin Schulz
  0 siblings, 0 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-11-20 13:20 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Andrew Lunn, Alexandre Belloni, davem, f.fainelli, allan.nielsen,
	linux-kernel, netdev, thomas.petazzoni, linux-riscv

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

Hi Andreas,

On Tue, Nov 20, 2018 at 12:39:51PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > I still don't see why that would cause a hang.
> 
> [  996.370000] [<ffffffe0011d7324>] mutex_lock+0x22/0x2a
> [  996.380000] [<ffffffe001039ba0>] mdiobus_read+0x36/0x60
> [  996.380000] [<ffffffe00103b82c>] vsc85xx_config_init+0x4c/0x1e2
> 
> vsc85xx_config_init is calling mdiobus_read while holding the mdio_lock.
> 

Thanks for reporting the bug. However, it could be nice the next time
you report a bug to give as much info as you can, such as the device
you're using. Architecture of your platform (or the board itself if it's
a DT in the Linux kernel), the MAC and the PHY IPs (the Microsemi PHY
driver now supports 6 different PHYs that do not work the same way so I
can't guess the one you're using), they all matter and help to
understand your bug. Also, if you could give a full bootlog of the
kernel and how it's possible to reproduce your issue (.config of your
kernel, on which commit on which branch you built your kernel).
Otherwise, you might not have any answer or considerably slow down the
bug identifying and fixing processes, which is annoying to you,
the users, the developers and the maintainers: a lot of upset people :)

Could you try something please? I'm pretty sure I found an issue (I
don't know if it's fixing your issue or not, but it might be).

Please replace in vsc85xx_default_config() the phy_read and phy_write by
respectively __phy_read and __phy_write (note the __ in front).

Let me know if you need help with this.

Thanks,
Quentin

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config
  2018-11-19 14:57   ` Andreas Schwab
  2018-11-19 15:10     ` Andrew Lunn
@ 2018-11-20 13:48     ` Andreas Schwab
  2018-11-20 13:55       ` Quentin Schulz
  1 sibling, 1 reply; 25+ messages in thread
From: Andreas Schwab @ 2018-11-20 13:48 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: davem, andrew, f.fainelli, allan.nielsen, linux-kernel, netdev,
	thomas.petazzoni, alexandre.belloni, linux-riscv

Don't use locking phy_read/phy_write functions while inside
phy_select_patch/phy_restore_page region.

Fixes: 6a0bfbbe20 ("net: phy: mscc: migrate to phy_select/restore_page functions")
Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 drivers/net/phy/mscc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index a2e59f4f6f..a398ab9410 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -814,10 +814,10 @@ static int vsc85xx_default_config(struct phy_device *phydev)
 	if (rc < 0)
 		goto out_unlock;
 
-	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
+	reg_val = __phy_read(phydev, MSCC_PHY_RGMII_CNTL);
 	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
 	reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
-	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
+	__phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
 
 out_unlock:
 	rc = phy_restore_page(phydev, rc, rc > 0 ? 0 : rc);
-- 
2.19.1


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config
  2018-11-20 13:48     ` [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config Andreas Schwab
@ 2018-11-20 13:55       ` Quentin Schulz
  2018-11-20 14:01         ` Andreas Schwab
  0 siblings, 1 reply; 25+ messages in thread
From: Quentin Schulz @ 2018-11-20 13:55 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: davem, andrew, f.fainelli, allan.nielsen, linux-kernel, netdev,
	thomas.petazzoni, alexandre.belloni, linux-riscv

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

Hi Andreas,

On Tue, Nov 20, 2018 at 02:48:37PM +0100, Andreas Schwab wrote:
> Don't use locking phy_read/phy_write functions while inside
> phy_select_patch/phy_restore_page region.
> 
> Fixes: 6a0bfbbe20 ("net: phy: mscc: migrate to phy_select/restore_page functions")
> Signed-off-by: Andreas Schwab <schwab@suse.de>

Have you tested this patch? Does it solve your issue?

Reviewed-by: Quentin Schulz <quentin.schulz@bootlin.com>
anyway since it's to be done.

You need to send your patch as a mail separate to this patch series.
You also need to prefix your patch with [PATCH net-next] instead of only
[PATCH]. This is specific to the net subsystem.

Use scripts/get_maintainer.pl on your patch to get the people to put in
Cc and in To.

Thanks,
Quentin

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config
  2018-11-20 13:55       ` Quentin Schulz
@ 2018-11-20 14:01         ` Andreas Schwab
  2018-11-20 14:17           ` Quentin Schulz
  0 siblings, 1 reply; 25+ messages in thread
From: Andreas Schwab @ 2018-11-20 14:01 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: davem, andrew, f.fainelli, allan.nielsen, linux-kernel, netdev,
	thomas.petazzoni, alexandre.belloni, linux-riscv

On Nov 20 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:

> You also need to prefix your patch with [PATCH net-next] instead of only
> [PATCH]. This is specific to the net subsystem.

Why next?  This is a bug fix that needs to go in now, not next.

> Use scripts/get_maintainer.pl on your patch to get the people to put in
> Cc and in To.

The same people as the original patch the added the bug.  Why would it
be different now?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config
  2018-11-20 14:01         ` Andreas Schwab
@ 2018-11-20 14:17           ` Quentin Schulz
  0 siblings, 0 replies; 25+ messages in thread
From: Quentin Schulz @ 2018-11-20 14:17 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: davem, andrew, f.fainelli, allan.nielsen, linux-kernel, netdev,
	thomas.petazzoni, alexandre.belloni, linux-riscv

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

Hi Andreas,

On Tue, Nov 20, 2018 at 03:01:25PM +0100, Andreas Schwab wrote:
> On Nov 20 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> 
> > You also need to prefix your patch with [PATCH net-next] instead of only
> > [PATCH]. This is specific to the net subsystem.
> 
> Why next?  This is a bug fix that needs to go in now, not next.
> 

Indeed. Use [PATCH net] then.
Please refer to https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

> > Use scripts/get_maintainer.pl on your patch to get the people to put in
> > Cc and in To.
> 
> The same people as the original patch the added the bug.  Why would it
> be different now?
> 

First, people may have changed mail addresses (especially maintainers)
and that is picked by get_maintainer.

Second, other people may have worked on this driver since the patch was
introduced and they might want to know about this change too and have
their word on it. This is what happened with this driver. Gustavo and
Heiner sent patches since then and they have already been merged.

This is also part of the contribution process which is defined here:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

You have not answered my question yet. Does this fix your issue, yes or
no?

Quentin

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2018-11-20 14:17 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-08 10:07 [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver Quentin Schulz
2018-10-08 10:07 ` [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions Quentin Schulz
2018-11-19 14:57   ` Andreas Schwab
2018-11-19 15:10     ` Andrew Lunn
2018-11-19 15:13       ` Andreas Schwab
2018-11-19 15:28         ` Andrew Lunn
2018-11-19 15:40           ` Alexandre Belloni
2018-11-19 15:50             ` Andreas Schwab
2018-11-19 16:12               ` Andrew Lunn
2018-11-19 16:14                 ` Andreas Schwab
2018-11-19 16:25                   ` Andrew Lunn
2018-11-19 16:32                     ` Andreas Schwab
2018-11-19 16:44                       ` Andrew Lunn
2018-11-20 11:39                 ` Andreas Schwab
2018-11-20 13:20                   ` Quentin Schulz
2018-11-20 13:48     ` [PATCH] net: phy: mscc: fix locking in vsc85xx_default_config Andreas Schwab
2018-11-20 13:55       ` Quentin Schulz
2018-11-20 14:01         ` Andreas Schwab
2018-11-20 14:17           ` Quentin Schulz
2018-10-08 10:07 ` [PATCH net-next v3 2/6] net: phy: mscc: add ethtool statistics counters Quentin Schulz
2018-10-08 10:07 ` [PATCH net-next v3 3/6] net: phy: mscc: Add EEE init sequence Quentin Schulz
2018-10-08 10:07 ` [PATCH net-next v3 4/6] net: phy: mscc: remove unneeded parenthesis Quentin Schulz
2018-10-08 10:07 ` [PATCH net-next v3 5/6] net: phy: mscc: shorten `x != 0` condition to `x` Quentin Schulz
2018-10-08 10:07 ` [PATCH net-next v3 6/6] net: phy: mscc: remove unneeded temporary variable Quentin Schulz
2018-10-08 17:29 ` [PATCH net-next v3 0/6] net: phy: mscc: various improvements to Microsemi PHY driver David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).