* Re: [PATCH net] net: phy: fix interrupt handling in non-started states
2019-02-12 18:56 [PATCH net] net: phy: fix interrupt handling in non-started states Heiner Kallweit
@ 2019-02-12 19:37 ` Andrew Lunn
2019-02-12 20:06 ` Heiner Kallweit
2019-02-14 1:17 ` Andrew Lunn
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2019-02-12 19:37 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org,
Russell King - ARM Linux
On Tue, Feb 12, 2019 at 07:56:15PM +0100, Heiner Kallweit wrote:
> phylib enables interrupts before phy_start() has been called, and if
> we receive an interrupt in a non-started state, the interrupt handler
> returns IRQ_NONE. This causes problems with at least one Marvell chip
> as reported by Andrew.
> Fix this by handling interrupts the same as in phy_mac_interrupt(),
> basically always running the phylib state machine. It knows when it
> has to do something and when not.
> This change allows to handle interrupts gracefully even if they
> occur in a non-started state.
>
> Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/phy.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 189cd2048c3a..ca5e0c0f018c 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -762,9 +762,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> {
> struct phy_device *phydev = phy_dat;
>
> - if (!phy_is_started(phydev))
> - return IRQ_NONE; /* It can't be ours. */
> -
> if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
> return IRQ_NONE;
Hi Heiner
Did you look at
ommit 3c3070d713d798f7f9e7ee3614e49b47655d14d8
Author: Maciej W. Rozycki <macro@linux-mips.org>
Date: Tue Oct 3 16:18:35 2006 +0100
[PATCH] 2.6.18: sb1250-mac: Phylib IRQ handling fixes
This patch fixes a couple of problems discovered with interrupt handling
in the phylib core, namely:
1. The driver uses timer and workqueue calls, but does not include
<linux/timer.h> nor <linux/workqueue.h>.
2. The driver uses schedule_work() for handling interrupts, but does not
make sure any pending work scheduled thus has been completed before
driver's structures get freed from memory. This is especially
important as interrupts may keep arriving if the line is shared with
another PHY.
The solution is to ignore phy_interrupt() calls if the reported device
has already been halted and calling flush_scheduled_work() from
phy_stop_interrupts() (but guarded with current_is_keventd() in case
the function has been called through keventd from the MAC device's
close call to avoid a deadlock on the netlink lock).
Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
patch-mips-2.6.18-20060920-phy-irq-16
Signed-off-by: Jeff Garzik <jeff@garzik.org>
There has been a lot of change since then, so maybe this is no longer
an issue?
Thanks
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: phy: fix interrupt handling in non-started states
2019-02-12 19:37 ` Andrew Lunn
@ 2019-02-12 20:06 ` Heiner Kallweit
0 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2019-02-12 20:06 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org,
Russell King - ARM Linux
On 12.02.2019 20:37, Andrew Lunn wrote:
> On Tue, Feb 12, 2019 at 07:56:15PM +0100, Heiner Kallweit wrote:
>> phylib enables interrupts before phy_start() has been called, and if
>> we receive an interrupt in a non-started state, the interrupt handler
>> returns IRQ_NONE. This causes problems with at least one Marvell chip
>> as reported by Andrew.
>> Fix this by handling interrupts the same as in phy_mac_interrupt(),
>> basically always running the phylib state machine. It knows when it
>> has to do something and when not.
>> This change allows to handle interrupts gracefully even if they
>> occur in a non-started state.
>>
>> Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/phy/phy.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 189cd2048c3a..ca5e0c0f018c 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -762,9 +762,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>> {
>> struct phy_device *phydev = phy_dat;
>>
>> - if (!phy_is_started(phydev))
>> - return IRQ_NONE; /* It can't be ours. */
>> -
>> if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
>> return IRQ_NONE;
>
> Hi Heiner
>
> Did you look at
>
Thanks for this interesting old read.
> ommit 3c3070d713d798f7f9e7ee3614e49b47655d14d8
> Author: Maciej W. Rozycki <macro@linux-mips.org>
> Date: Tue Oct 3 16:18:35 2006 +0100
>
> [PATCH] 2.6.18: sb1250-mac: Phylib IRQ handling fixes
>
> This patch fixes a couple of problems discovered with interrupt handling
> in the phylib core, namely:
>
> 1. The driver uses timer and workqueue calls, but does not include
> <linux/timer.h> nor <linux/workqueue.h>.
>
> 2. The driver uses schedule_work() for handling interrupts, but does not
> make sure any pending work scheduled thus has been completed before
> driver's structures get freed from memory. This is especially
> important as interrupts may keep arriving if the line is shared with
> another PHY.
>
Since that time this have massively changed and I *think* this doesn't apply
any longer. cancel_delayed_work_sync() is called always before driver
structure is freed.
> The solution is to ignore phy_interrupt() calls if the reported device
> has already been halted and calling flush_scheduled_work() from
> phy_stop_interrupts() (but guarded with current_is_keventd() in case
> the function has been called through keventd from the MAC device's
> close call to avoid a deadlock on the netlink lock).
>
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
>
> patch-mips-2.6.18-20060920-phy-irq-16
> Signed-off-by: Jeff Garzik <jeff@garzik.org>
>
> There has been a lot of change since then, so maybe this is no longer
> an issue?
>
AFAICS, yes. I'll have a closer look at the scenario described by Russell,
this however should be independent of the patch here.
> Thanks
> Andrew
>
Heiner
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: phy: fix interrupt handling in non-started states
2019-02-12 18:56 [PATCH net] net: phy: fix interrupt handling in non-started states Heiner Kallweit
2019-02-12 19:37 ` Andrew Lunn
@ 2019-02-14 1:17 ` Andrew Lunn
2019-02-14 4:10 ` Florian Fainelli
2019-02-14 4:44 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2019-02-14 1:17 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org,
Russell King - ARM Linux
On Tue, Feb 12, 2019 at 07:56:15PM +0100, Heiner Kallweit wrote:
> phylib enables interrupts before phy_start() has been called, and if
> we receive an interrupt in a non-started state, the interrupt handler
> returns IRQ_NONE. This causes problems with at least one Marvell chip
> as reported by Andrew.
> Fix this by handling interrupts the same as in phy_mac_interrupt(),
> basically always running the phylib state machine. It knows when it
> has to do something and when not.
> This change allows to handle interrupts gracefully even if they
> occur in a non-started state.
>
> Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: phy: fix interrupt handling in non-started states
2019-02-12 18:56 [PATCH net] net: phy: fix interrupt handling in non-started states Heiner Kallweit
2019-02-12 19:37 ` Andrew Lunn
2019-02-14 1:17 ` Andrew Lunn
@ 2019-02-14 4:10 ` Florian Fainelli
2019-02-14 4:44 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2019-02-14 4:10 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, David Miller
Cc: netdev@vger.kernel.org, Russell King - ARM Linux
On 2/12/2019 10:56 AM, Heiner Kallweit wrote:
> phylib enables interrupts before phy_start() has been called, and if
> we receive an interrupt in a non-started state, the interrupt handler
> returns IRQ_NONE. This causes problems with at least one Marvell chip
> as reported by Andrew.
> Fix this by handling interrupts the same as in phy_mac_interrupt(),
> basically always running the phylib state machine. It knows when it
> has to do something and when not.
> This change allows to handle interrupts gracefully even if they
> occur in a non-started state.
>
> Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: phy: fix interrupt handling in non-started states
2019-02-12 18:56 [PATCH net] net: phy: fix interrupt handling in non-started states Heiner Kallweit
` (2 preceding siblings ...)
2019-02-14 4:10 ` Florian Fainelli
@ 2019-02-14 4:44 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2019-02-14 4:44 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, netdev, linux
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 12 Feb 2019 19:56:15 +0100
> phylib enables interrupts before phy_start() has been called, and if
> we receive an interrupt in a non-started state, the interrupt handler
> returns IRQ_NONE. This causes problems with at least one Marvell chip
> as reported by Andrew.
> Fix this by handling interrupts the same as in phy_mac_interrupt(),
> basically always running the phylib state machine. It knows when it
> has to do something and when not.
> This change allows to handle interrupts gracefully even if they
> occur in a non-started state.
>
> Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Applied, thanks Heiner.
^ permalink raw reply [flat|nested] 6+ messages in thread