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

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

[Heiner Kallweit]

This patch series improves few aspects of starting the PHY.

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        | 52 +++++++++++++++++-------------------
 drivers/net/phy/phy_device.c |  5 ++--
 drivers/net/phy/phylink.c    |  5 ++--
 include/linux/phy.h          |  2 +-
 4 files changed, 29 insertions(+), 35 deletions(-)

-- 
2.20.1

[PATCH net-next 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 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.

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 3/4] net: phy: enable 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 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.

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 30ba650bb..7bbe8bf8a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -790,28 +790,23 @@ 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);
 		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 4/4] net: phy: change phy_start_interrupts to phy_request_interrupt

[Florian Fainelli]

On January 19, 2019 3:30:05 AM PST, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>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.
>
>Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>---

>+ * 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);
> 		phydev->irq = PHY_POLL;
>-		return 0;
> 	}

We should propagate the return code here and/or indicate we are falling back to polling since may not be desired. An use case that should be considered is probe deferral for instance.
-- 
Florian

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

[Florian Fainelli]

Hi Heiner,

On January 19, 2019 3:27:29 AM PST, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>This patch series improves few aspects of starting the PHY.

This looks good as usual, just one minor comment in patch 4. Thanks!

>
>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        | 52 +++++++++++++++++-------------------
> drivers/net/phy/phy_device.c |  5 ++--
> drivers/net/phy/phylink.c    |  5 ++--
> include/linux/phy.h          |  2 +-
> 4 files changed, 29 insertions(+), 35 deletions(-)

-- 
Florian

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

[Heiner Kallweit]

On 20.01.2019 00:43, Florian Fainelli wrote:
> 
> 
> On January 19, 2019 3:30:05 AM PST, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> 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.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
> 
>> + * 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);
>> 		phydev->irq = PHY_POLL;
>> -		return 0;
>> 	}
> 
> We should propagate the return code here and/or indicate we are falling back to polling since may not be desired. An use case that should be considered is probe deferral for instance.
> 
I kept the current behavior to basically ignore the error and just
warn and fall back to polling. Seems that phylib has behaved this way
forever. If we come to the conclusion that changing this behavior
makes sense, shouldn't we do it separately? Also we may break systems
relying on the fallback to polling.
But something we can easily agree on is to extend the warning
to state that we fall back to polling.