* [PATCH v2 net-next 0/3] net: phy: improve stopping PHY
@ 2019-01-16 20:24 Heiner Kallweit
2019-01-16 20:25 ` [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect Heiner Kallweit
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Heiner Kallweit @ 2019-01-16 20:24 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect
2019-01-16 20:24 [PATCH v2 net-next 0/3] net: phy: improve stopping PHY Heiner Kallweit
@ 2019-01-16 20:25 ` Heiner Kallweit
2019-01-16 21:48 ` Andrew Lunn
2019-01-16 20:25 ` [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop Heiner Kallweit
2019-01-16 20:26 ` [PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts Heiner Kallweit
2 siblings, 1 reply; 10+ messages in thread
From: Heiner Kallweit @ 2019-01-16 20:25 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop
2019-01-16 20:24 [PATCH v2 net-next 0/3] net: phy: improve stopping PHY Heiner Kallweit
2019-01-16 20:25 ` [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect Heiner Kallweit
@ 2019-01-16 20:25 ` Heiner Kallweit
2019-01-16 21:59 ` Andrew Lunn
2019-01-16 20:26 ` [PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts Heiner Kallweit
2 siblings, 1 reply; 10+ messages in thread
From: Heiner Kallweit @ 2019-01-16 20:25 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts
2019-01-16 20:24 [PATCH v2 net-next 0/3] net: phy: improve stopping PHY Heiner Kallweit
2019-01-16 20:25 ` [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect Heiner Kallweit
2019-01-16 20:25 ` [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop Heiner Kallweit
@ 2019-01-16 20:26 ` Heiner Kallweit
2019-01-16 22:00 ` Andrew Lunn
2 siblings, 1 reply; 10+ messages in thread
From: Heiner Kallweit @ 2019-01-16 20:26 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect
2019-01-16 20:25 ` [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect Heiner Kallweit
@ 2019-01-16 21:48 ` Andrew Lunn
2019-01-17 6:19 ` Heiner Kallweit
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2019-01-16 21:48 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop
2019-01-16 20:25 ` [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop Heiner Kallweit
@ 2019-01-16 21:59 ` Andrew Lunn
2019-01-17 6:10 ` Heiner Kallweit
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2019-01-16 21:59 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts
2019-01-16 20:26 ` [PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts Heiner Kallweit
@ 2019-01-16 22:00 ` Andrew Lunn
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2019-01-16 22:00 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop
2019-01-16 21:59 ` Andrew Lunn
@ 2019-01-17 6:10 ` Heiner Kallweit
0 siblings, 0 replies; 10+ messages in thread
From: Heiner Kallweit @ 2019-01-17 6:10 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect
2019-01-16 21:48 ` Andrew Lunn
@ 2019-01-17 6:19 ` Heiner Kallweit
2019-01-17 18:55 ` Heiner Kallweit
0 siblings, 1 reply; 10+ messages in thread
From: Heiner Kallweit @ 2019-01-17 6:19 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect
2019-01-17 6:19 ` Heiner Kallweit
@ 2019-01-17 18:55 ` Heiner Kallweit
0 siblings, 0 replies; 10+ messages in thread
From: Heiner Kallweit @ 2019-01-17 18:55 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-17 18:56 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16 20:24 [PATCH v2 net-next 0/3] net: phy: improve stopping PHY Heiner Kallweit
2019-01-16 20:25 ` [PATCH v2 net-next 1/3] net: phy: check that PHY is stopped when entering phy_disconnect Heiner Kallweit
2019-01-16 21:48 ` Andrew Lunn
2019-01-17 6:19 ` Heiner Kallweit
2019-01-17 18:55 ` Heiner Kallweit
2019-01-16 20:25 ` [PATCH v2 net-next 2/3] net: phy: ensure phylib state machine is stopped after calling phy_stop Heiner Kallweit
2019-01-16 21:59 ` Andrew Lunn
2019-01-17 6:10 ` Heiner Kallweit
2019-01-16 20:26 ` [PATCH v2 net-next 3/3] net: phy: remove phy_stop_interrupts Heiner Kallweit
2019-01-16 22:00 ` Andrew Lunn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.