[PATCH net-next v3 0/4] net: phy: improve starting PHY

[PATCH net-next v3 0/4] net: phy: improve starting PHY

[Heiner Kallweit]

This patch series improves few aspects of starting the PHY.

v2:
- improve a warning in patch 4
v3:
- extend commit message for patch 2

Heiner Kallweit (4):
  net: phy: start state machine in phy_start only
  net: phy: warn if phy_start is called from invalid state
  net: phy: start interrupts in phy_start
  net: phy: change phy_start_interrupts to phy_request_interrupt

 drivers/net/phy/phy.c        | 64 ++++++++++++++++++------------------
 drivers/net/phy/phy_device.c |  5 ++-
 drivers/net/phy/phylink.c    |  5 ++-
 include/linux/phy.h          |  2 +-
 4 files changed, 37 insertions(+), 39 deletions(-)

-- 
2.20.1

[PATCH net-next v3 1/4] net: phy: start state machine in phy_start only

[Heiner Kallweit]

The state machine is a no-op before phy_start() has been called.
Therefore let's enable it in phy_start() only. In phy_start()
let's call phy_start_machine() instead of phy_trigger_machine().
phy_start_machine is an alias for phy_trigger_machine but it makes
clearer that we start the state machine here instead of just
triggering a run.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c        | 2 +-
 drivers/net/phy/phy_device.c | 1 -
 drivers/net/phy/phylink.c    | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 745a705a5..3df6aadc5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -883,7 +883,7 @@ void phy_start(struct phy_device *phydev)
 	}
 	mutex_unlock(&phydev->lock);
 
-	phy_trigger_machine(phydev);
+	phy_start_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e0ceecbed..3e284d596 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -944,7 +944,6 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
 		return rc;
 
 	phy_prepare_link(phydev, handler);
-	phy_start_machine(phydev);
 	if (phydev->irq > 0)
 		phy_start_interrupts(phydev);
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e7becc737..e9b8f1037 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -679,7 +679,6 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising);
 
-	phy_start_machine(phy);
 	if (phy->irq > 0)
 		phy_start_interrupts(phy);
 
-- 
2.20.1

[PATCH net-next v3 2/4] net: phy: warn if phy_start is called from invalid state

[Heiner Kallweit]

phy_start() should be called from states PHY_READY or PHY_HALTED only.
Check for this to detect misbehaving drivers. Also the state machine
should be started only when being called from one of the valid states.

Some more background:
For all invalid states phy_start() basically was a no-op. All it did
was triggering a state machine run, but for all "running" states the
poll loop was active anyway. And if called from PHY_DOWN, the state
machine does nothing.

v3:
- extended commit message

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3df6aadc5..fd928979b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -861,9 +861,16 @@ void phy_start(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
+	if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
+		WARN(1, "called from state %s\n",
+		     phy_state_to_str(phydev->state));
+		goto out;
+	}
+
 	switch (phydev->state) {
 	case PHY_READY:
 		phydev->state = PHY_UP;
+		phy_start_machine(phydev);
 		break;
 	case PHY_HALTED:
 		/* if phy was suspended, bring the physical link up again */
@@ -877,13 +884,13 @@ void phy_start(struct phy_device *phydev)
 		}
 
 		phydev->state = PHY_RESUMING;
+		phy_start_machine(phydev);
 		break;
 	default:
 		break;
 	}
+out:
 	mutex_unlock(&phydev->lock);
-
-	phy_start_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);
 
-- 
2.20.1

[PATCH net-next v3 3/4] net: phy: start interrupts in phy_start

[Heiner Kallweit]

Interrupts don't have to be enabled before calling phy_start().
Therefore let's enable them in phy_start(). In a subsequent step
we'll remove enabling interrupts from phy_connect_direct().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fd928979b..30ba650bb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -857,7 +857,7 @@ EXPORT_SYMBOL(phy_stop);
  */
 void phy_start(struct phy_device *phydev)
 {
-	int err = 0;
+	int err;
 
 	mutex_lock(&phydev->lock);
 
@@ -867,28 +867,22 @@ void phy_start(struct phy_device *phydev)
 		goto out;
 	}
 
-	switch (phydev->state) {
-	case PHY_READY:
-		phydev->state = PHY_UP;
-		phy_start_machine(phydev);
-		break;
-	case PHY_HALTED:
-		/* if phy was suspended, bring the physical link up again */
-		__phy_resume(phydev);
+	/* if phy was suspended, bring the physical link up again */
+	__phy_resume(phydev);
 
-		/* make sure interrupts are re-enabled for the PHY */
-		if (phy_interrupt_is_valid(phydev)) {
-			err = phy_enable_interrupts(phydev);
-			if (err < 0)
-				break;
-		}
+	/* make sure interrupts are enabled for the PHY */
+	if (phy_interrupt_is_valid(phydev)) {
+		err = phy_enable_interrupts(phydev);
+		if (err < 0)
+			goto out;
+	}
 
+	if (phydev->state == PHY_READY)
+		phydev->state = PHY_UP;
+	else
 		phydev->state = PHY_RESUMING;
-		phy_start_machine(phydev);
-		break;
-	default:
-		break;
-	}
+
+	phy_start_machine(phydev);
 out:
 	mutex_unlock(&phydev->lock);
 }
-- 
2.20.1

[PATCH net-next v3 4/4] net: phy: change phy_start_interrupts to phy_request_interrupt

[Heiner Kallweit]

Now that we enable the interrupts in phy_start() we don't have to do it
before. Therefore remove enabling interrupts from phy_start_interrupts()
and rename this function to reflect the changed functionality.

v2:
- improve warning to clearly state that we fall back to polling

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c        | 23 +++++++++++------------
 drivers/net/phy/phy_device.c |  4 ++--
 drivers/net/phy/phylink.c    |  4 ++--
 include/linux/phy.h          |  2 +-
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 30ba650bb..58ce3dac2 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -790,28 +790,27 @@ static int phy_enable_interrupts(struct phy_device *phydev)
 }
 
 /**
- * phy_start_interrupts - request and enable interrupts for a PHY device
+ * phy_request_interrupt - request interrupt for a PHY device
  * @phydev: target phy_device struct
  *
  * Description: Request the interrupt for the given PHY.
  *   If this fails, then we set irq to PHY_POLL.
- *   Otherwise, we enable the interrupts in the PHY.
  *   This should only be called with a valid IRQ number.
- *   Returns 0 on success or < 0 on error.
  */
-int phy_start_interrupts(struct phy_device *phydev)
+void phy_request_interrupt(struct phy_device *phydev)
 {
-	if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
-				 IRQF_ONESHOT | IRQF_SHARED,
-				 phydev_name(phydev), phydev) < 0) {
-		phydev_warn(phydev, "Can't get IRQ %d\n", phydev->irq);
+	int err;
+
+	err = request_threaded_irq(phydev->irq, NULL, phy_interrupt,
+				   IRQF_ONESHOT | IRQF_SHARED,
+				   phydev_name(phydev), phydev);
+	if (err) {
+		phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n",
+			    err, phydev->irq);
 		phydev->irq = PHY_POLL;
-		return 0;
 	}
-
-	return phy_enable_interrupts(phydev);
 }
-EXPORT_SYMBOL(phy_start_interrupts);
+EXPORT_SYMBOL(phy_request_interrupt);
 
 /**
  * phy_stop - Bring down the PHY link, and stop checking the status
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e284d596..dd3a2302f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -944,8 +944,8 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
 		return rc;
 
 	phy_prepare_link(phydev, handler);
-	if (phydev->irq > 0)
-		phy_start_interrupts(phydev);
+	if (phy_interrupt_is_valid(phydev))
+		phy_request_interrupt(phydev);
 
 	return 0;
 }
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e9b8f1037..1ac832ba0 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -679,8 +679,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising);
 
-	if (phy->irq > 0)
-		phy_start_interrupts(phy);
+	if (phy_interrupt_is_valid(phy))
+		phy_request_interrupt(phy);
 
 	return 0;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1fa7c367b..d0055adbc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1045,7 +1045,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			      const struct ethtool_link_ksettings *cmd);
 int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd);
-int phy_start_interrupts(struct phy_device *phydev);
+void phy_request_interrupt(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
 int phy_set_max_speed(struct phy_device *phydev, u32 max_speed);
 void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);
-- 
2.20.1

Re: [PATCH net-next v3 0/4] net: phy: improve starting PHY

[S-k, Shyam-sundar]

On 1/23/2019 11:55 AM, Heiner Kallweit wrote:
> This patch series improves few aspects of starting the PHY.
>
> v2:
> - improve a warning in patch 4
> v3:
> - extend commit message for patch 2
>
> Heiner Kallweit (4):
>   net: phy: start state machine in phy_start only
>   net: phy: warn if phy_start is called from invalid state
>   net: phy: start interrupts in phy_start
>   net: phy: change phy_start_interrupts to phy_request_interrupt
>
>  drivers/net/phy/phy.c        | 64 ++++++++++++++++++------------------
>  drivers/net/phy/phy_device.c |  5 ++-
>  drivers/net/phy/phylink.c    |  5 ++-
>  include/linux/phy.h          |  2 +-
>  4 files changed, 37 insertions(+), 39 deletions(-)
Changes works well on AMD xgbe hardware.

Tested-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Re: [PATCH net-next v3 0/4] net: phy: improve starting PHY

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Wed, 23 Jan 2019 07:25:38 +0100

> This patch series improves few aspects of starting the PHY.
> 
> v2:
> - improve a warning in patch 4
> v3:
> - extend commit message for patch 2

Series applied, thanks Heiner.