[PATCH v2 net-next 0/3] net: phy: improve stopping PHY

[PATCH v2 net-next 0/3] net: phy: improve stopping PHY

[Heiner Kallweit]

This patchset improves and simplifies stopping the PHY.

Heiner Kallweit (3):
  net: phy: check that PHY is stopped when entering phy_disconnect
  net: phy: ensure phylib state machine is stopped after calling phy_stop
  net: phy: remove phy_stop_interrupts

v2:
- break down the patch to a patchset

 drivers/net/phy/phy.c        | 18 +-----------------
 drivers/net/phy/phy_device.c |  9 ++++++---
 include/linux/phy.h          |  1 -
 3 files changed, 7 insertions(+), 21 deletions(-)

-- 
2.20.1

[PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect

[Heiner Kallweit]

Every driver should have called phy_stop() before calling
phy_disconnect(). Let's check for this and ensure PHY is stopped
when starting with the actual work in phy_disconnect().

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b5f5cda4c..46ec71021 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -999,6 +999,11 @@ EXPORT_SYMBOL(phy_connect);
  */
 void phy_disconnect(struct phy_device *phydev)
 {
+	if (phy_is_started(phydev)) {
+		phydev_warn(phydev, "phy_stop should have been called before phy_disconnect!\n");
+		phy_stop(phydev);
+	}
+
 	if (phydev->irq > 0)
 		phy_stop_interrupts(phydev);
 
-- 
2.20.1

Re: [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect

[Andrew Lunn]

On Wed, Jan 16, 2019 at 09:25:15PM +0100, Heiner Kallweit wrote:
> Every driver should have called phy_stop() before calling
> phy_disconnect(). Let's check for this and ensure PHY is stopped
> when starting with the actual work in phy_disconnect().

Hi Heiner

Looking at the patch, i think why must the MAC driver call phy_stop()
before phy_disconnect()? It keeps is symmetrical, you need
phy_connect() and then phy_start(). But if the core can detect that
phy_stop() has not been called, and can call phy_stop() when needed,
we can probably simplify the MAC drivers by removing many of the
phy_stop() calls.

I think it might come down to where the phy_connect()/phy_disconnect()
is performed. Sometimes it is in probe()/remove(), sometimes it is in
open()/close(). If phy_disconnect() is in remove(), phy_stop() is
needed in close(). But if phy_disconnect() is called in close() the
phy_stop() could be skipped?

Before we start adding warning, we probably should first document the
expectations. Documentation/networking/phy.txt seems like a good
place.

	Andrew

Re: [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect

[Heiner Kallweit]

On 16.01.2019 22:48, Andrew Lunn wrote:
> On Wed, Jan 16, 2019 at 09:25:15PM +0100, Heiner Kallweit wrote:
>> Every driver should have called phy_stop() before calling
>> phy_disconnect(). Let's check for this and ensure PHY is stopped
>> when starting with the actual work in phy_disconnect().
> 
> Hi Heiner
> 
> Looking at the patch, i think why must the MAC driver call phy_stop()
> before phy_disconnect()? It keeps is symmetrical, you need
> phy_connect() and then phy_start(). But if the core can detect that
> phy_stop() has not been called, and can call phy_stop() when needed,
> we can probably simplify the MAC drivers by removing many of the
> phy_stop() calls.
> 
> I think it might come down to where the phy_connect()/phy_disconnect()
> is performed. Sometimes it is in probe()/remove(), sometimes it is in
> open()/close(). If phy_disconnect() is in remove(), phy_stop() is
> needed in close(). But if phy_disconnect() is called in close() the
> phy_stop() could be skipped?
> 
Right, there may be cases where this is possible. However typically I
see the following in network drivers in close():
- first different things are stopped / cleaned up, where order is
  critical (stopping PHY, tx queues, ..)
- then resources are released (free interrupt, ..)
Therefore I assume that in most cases the split to phy_stop() and
phy_disconnect() is needed.

> Before we start adding warning, we probably should first document the
> expectations. Documentation/networking/phy.txt seems like a good
> place.
> 
Indeed, that's something we should do.

> 	Andrew
> 
Heiner

Re: [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect

[Heiner Kallweit]

On 17.01.2019 07:19, Heiner Kallweit wrote:
> On 16.01.2019 22:48, Andrew Lunn wrote:
>> On Wed, Jan 16, 2019 at 09:25:15PM +0100, Heiner Kallweit wrote:
>>> Every driver should have called phy_stop() before calling
>>> phy_disconnect(). Let's check for this and ensure PHY is stopped
>>> when starting with the actual work in phy_disconnect().
>>
>> Hi Heiner
>>
>> Looking at the patch, i think why must the MAC driver call phy_stop()
>> before phy_disconnect()? It keeps is symmetrical, you need
>> phy_connect() and then phy_start(). But if the core can detect that
>> phy_stop() has not been called, and can call phy_stop() when needed,
>> we can probably simplify the MAC drivers by removing many of the
>> phy_stop() calls.
>>
>> I think it might come down to where the phy_connect()/phy_disconnect()
>> is performed. Sometimes it is in probe()/remove(), sometimes it is in
>> open()/close(). If phy_disconnect() is in remove(), phy_stop() is
>> needed in close(). But if phy_disconnect() is called in close() the
>> phy_stop() could be skipped?
>>
> Right, there may be cases where this is possible. However typically I
> see the following in network drivers in close():
> - first different things are stopped / cleaned up, where order is
>   critical (stopping PHY, tx queues, ..)
> - then resources are released (free interrupt, ..)
> Therefore I assume that in most cases the split to phy_stop() and
> phy_disconnect() is needed.
> 
After thinking about it again I think I'll go with the proposed
approach to leave it to the driver whether he wants to call phy_stop()
separately or not. So I will just remove the warning.

>> Before we start adding warning, we probably should first document the
>> expectations. Documentation/networking/phy.txt seems like a good
>> place.
>>
> Indeed, that's something we should do.
> 
>> 	Andrew
>>
> Heiner
> 

[PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop

[Heiner Kallweit]

The call to the phylib state machine in phy_stop() just ensures that
the state machine isn't re-triggered, but a state machine call may
be scheduled already. So lets's call phy_stop_machine().
This also allows to get rid of the call to phy_stop_machine() in
phy_disconnect().

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2ffe08537..96e9ec252 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -853,6 +853,7 @@ void phy_stop(struct phy_device *phydev)
 	mutex_unlock(&phydev->lock);
 
 	phy_state_machine(&phydev->state_queue.work);
+	phy_stop_machine(phydev);
 
 	/* Cannot call flush_scheduled_work() here as desired because
 	 * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 46ec71021..60cd976f4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1007,8 +1007,6 @@ void phy_disconnect(struct phy_device *phydev)
 	if (phydev->irq > 0)
 		phy_stop_interrupts(phydev);
 
-	phy_stop_machine(phydev);
-
 	phydev->adjust_link = NULL;
 
 	phy_detach(phydev);
-- 
2.20.1

Re: [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop

[Andrew Lunn]

On Wed, Jan 16, 2019 at 09:25:54PM +0100, Heiner Kallweit wrote:
> The call to the phylib state machine in phy_stop() just ensures that
> the state machine isn't re-triggered, but a state machine call may
> be scheduled already. So lets's call phy_stop_machine().
> This also allows to get rid of the call to phy_stop_machine() in
> phy_disconnect().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

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

At some point it would be good to audit the code and see if anything
is still used in interrupt context. PHY interrupt handling is now done
in a threaded interrupt handler. So i think it is just callers to
phy_mac_interrupt() that need to be checked. It could be the work
queue can be removed and everything done synchronous.

    Andrew

Re: [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop

[Heiner Kallweit]

On 16.01.2019 22:59, Andrew Lunn wrote:
> On Wed, Jan 16, 2019 at 09:25:54PM +0100, Heiner Kallweit wrote:
>> The call to the phylib state machine in phy_stop() just ensures that
>> the state machine isn't re-triggered, but a state machine call may
>> be scheduled already. So lets's call phy_stop_machine().
>> This also allows to get rid of the call to phy_stop_machine() in
>> phy_disconnect().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> At some point it would be good to audit the code and see if anything
> is still used in interrupt context. PHY interrupt handling is now done
> in a threaded interrupt handler. So i think it is just callers to
> phy_mac_interrupt() that need to be checked. It could be the work
> queue can be removed and everything done synchronous.
> 
Yes, I think few calls to phy_trigger_machine() could be changed to be
synchronous. But I think the workqueue will still  be needed:
- for phy_mac_interrupt() which is typically called from hard irq
  context
- for polling link status (delayed work)

>     Andrew
> 
Heiner

[PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts

[Heiner Kallweit]

Interrupts have been disabled in phy_stop() already. So we can remove
phy_stop_interrupts() and free the interrupt in phy_disconnect()
directly.

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 96e9ec252..745a705a5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -813,23 +813,6 @@ int phy_start_interrupts(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start_interrupts);
 
-/**
- * phy_stop_interrupts - disable interrupts from a PHY device
- * @phydev: target phy_device struct
- */
-int phy_stop_interrupts(struct phy_device *phydev)
-{
-	int err = phy_disable_interrupts(phydev);
-
-	if (err)
-		phy_error(phydev);
-
-	free_irq(phydev->irq, phydev);
-
-	return err;
-}
-EXPORT_SYMBOL(phy_stop_interrupts);
-
 /**
  * phy_stop - Bring down the PHY link, and stop checking the status
  * @phydev: target phy_device struct
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 60cd976f4..edcb49ee9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1004,8 +1004,8 @@ void phy_disconnect(struct phy_device *phydev)
 		phy_stop(phydev);
 	}
 
-	if (phydev->irq > 0)
-		phy_stop_interrupts(phydev);
+	if (phy_interrupt_is_valid(phydev))
+		free_irq(phydev->irq, phydev);
 
 	phydev->adjust_link = NULL;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 00b0533de..1fa7c367b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -951,7 +951,6 @@ int phy_aneg_done(struct phy_device *phydev);
 int phy_speed_down(struct phy_device *phydev, bool sync);
 int phy_speed_up(struct phy_device *phydev);
 
-int phy_stop_interrupts(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
 int phy_reset_after_clk_enable(struct phy_device *phydev);
 
-- 
2.20.1

Re: [PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts

[Andrew Lunn]

On Wed, Jan 16, 2019 at 09:26:44PM +0100, Heiner Kallweit wrote:
> Interrupts have been disabled in phy_stop() already. So we can remove
> phy_stop_interrupts() and free the interrupt in phy_disconnect()
> directly.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

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

    Andrew