* Re: [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt
2018-03-07 22:50 [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt Brad Mouring
@ 2018-03-08 16:29 ` Andrew Lunn
2018-03-08 16:46 ` Brad Mouring
2018-03-08 19:41 ` Sergei Shtylyov
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2018-03-08 16:29 UTC (permalink / raw)
To: Brad Mouring; +Cc: Florian Fainelli, netdev, linux-kernel
On Wed, Mar 07, 2018 at 04:50:42PM -0600, Brad Mouring wrote:
> If multiple phys share the same interrupt (e.g. a multi-phy chip),
> the first device registered is the only one checked as phy_interrupt
> will always return IRQ_HANDLED if the first phydev is not halted.
> Move the interrupt check into phy_interrupt and, if it was not this
> phydev, return IRQ_NONE to allow other devices on this irq a chance
> to check if it was their interrupt.
>
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> ---
> drivers/net/phy/phy.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e3e29c2b028b..ff1aa815568f 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> if (PHY_HALTED == phydev->state)
> return IRQ_NONE; /* It can't be ours. */
>
> + if (phy_interrupt_is_valid(phydev)) {
Hi Brad
Is this check needed? Can phy_interrupt() be called for a PHY which
does not have an interrupt?
> + if (phydev->drv->did_interrupt &&
> + !phydev->drv->did_interrupt(phydev))
> + return IRQ_NONE;
> + }
> +
> phy_change(phydev);
>
> return IRQ_HANDLED;
> @@ -725,16 +731,6 @@ EXPORT_SYMBOL(phy_stop_interrupts);
> */
> void phy_change(struct phy_device *phydev)
> {
> - if (phy_interrupt_is_valid(phydev)) {
> - if (phydev->drv->did_interrupt &&
> - !phydev->drv->did_interrupt(phydev))
> - return;
> -
> - if (phydev->state == PHY_HALTED)
> - if (phy_disable_interrupts(phydev))
> - goto phy_err;
> - }
> -
phy_change() can also be called via phy_mac_interrupt(). I wonder if
this change is going to break anything? Did you think about that?
Thanks
Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt
2018-03-08 16:29 ` Andrew Lunn
@ 2018-03-08 16:46 ` Brad Mouring
0 siblings, 0 replies; 11+ messages in thread
From: Brad Mouring @ 2018-03-08 16:46 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev, linux-kernel
On Thu, Mar 08, 2018 at 05:29:05PM +0100, Andrew Lunn wrote:
> On Wed, Mar 07, 2018 at 04:50:42PM -0600, Brad Mouring wrote:
> > If multiple phys share the same interrupt (e.g. a multi-phy chip),
> > the first device registered is the only one checked as phy_interrupt
> > will always return IRQ_HANDLED if the first phydev is not halted.
> > Move the interrupt check into phy_interrupt and, if it was not this
> > phydev, return IRQ_NONE to allow other devices on this irq a chance
> > to check if it was their interrupt.
> >
> > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > ---
> > drivers/net/phy/phy.c | 16 ++++++----------
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index e3e29c2b028b..ff1aa815568f 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> > if (PHY_HALTED == phydev->state)
> > return IRQ_NONE; /* It can't be ours. */
> >
> > + if (phy_interrupt_is_valid(phydev)) {
>
> Hi Brad
>
> Is this check needed? Can phy_interrupt() be called for a PHY which
> does not have an interrupt?
Ah, fair point (and I guess a result of copy-pasting without
thinking), to address this and the next point...
> > + if (phydev->drv->did_interrupt &&
> > + !phydev->drv->did_interrupt(phydev))
> > + return IRQ_NONE;
> > + }
> > +
> > phy_change(phydev);
> >
> > return IRQ_HANDLED;
> > @@ -725,16 +731,6 @@ EXPORT_SYMBOL(phy_stop_interrupts);
> > */
> > void phy_change(struct phy_device *phydev)
> > {
> > - if (phy_interrupt_is_valid(phydev)) {
> > - if (phydev->drv->did_interrupt &&
> > - !phydev->drv->did_interrupt(phydev))
> > - return;
> > -
> > - if (phydev->state == PHY_HALTED)
> > - if (phy_disable_interrupts(phydev))
> > - goto phy_err;
> > - }
> > -
>
> phy_change() can also be called via phy_mac_interrupt(). I wonder if
> this change is going to break anything? Did you think about that?
Thanks for pointing this out. I'll post a v2 that (hopefully) address
both of these valid points.
Thanks,
Brad
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt
2018-03-07 22:50 [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt Brad Mouring
2018-03-08 16:29 ` Andrew Lunn
@ 2018-03-08 19:41 ` Sergei Shtylyov
2018-03-08 19:57 ` Sergei Shtylyov
2018-03-08 20:16 ` Brad Mouring
2018-03-08 20:25 ` Sergei Shtylyov
2018-03-08 20:31 ` [PATCH] net: phy: Tell caller result of phy_change() Brad Mouring
3 siblings, 2 replies; 11+ messages in thread
From: Sergei Shtylyov @ 2018-03-08 19:41 UTC (permalink / raw)
To: Brad Mouring, Andrew Lunn, Florian Fainelli; +Cc: netdev, linux-kernel
Hello!
On 03/08/2018 01:50 AM, Brad Mouring wrote:
> If multiple phys share the same interrupt (e.g. a multi-phy chip),
> the first device registered is the only one checked as phy_interrupt
> will always return IRQ_HANDLED if the first phydev is not halted.
> Move the interrupt check into phy_interrupt and, if it was not this
> phydev, return IRQ_NONE to allow other devices on this irq a chance
> to check if it was their interrupt.
Hm, looking at kernel/irq/handle.c, all registered IRQ handlers are always
called regardless of their results. Care to explain?
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> ---
> drivers/net/phy/phy.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e3e29c2b028b..ff1aa815568f 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> if (PHY_HALTED == phydev->state)
> return IRQ_NONE; /* It can't be ours. */
>
> + if (phy_interrupt_is_valid(phydev)) {
Always true in this context, no?
> + if (phydev->drv->did_interrupt &&
> + !phydev->drv->did_interrupt(phydev))
I don't think we can do this in the interrupt context as this function *will*
read from MDIO... I think that was the reason why IRQ handling is done in the
thread context...
> + return IRQ_NONE;
> + }
> +
> phy_change(phydev);
>
> return IRQ_HANDLED;
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt
2018-03-08 19:41 ` Sergei Shtylyov
@ 2018-03-08 19:57 ` Sergei Shtylyov
2018-03-08 20:16 ` Brad Mouring
1 sibling, 0 replies; 11+ messages in thread
From: Sergei Shtylyov @ 2018-03-08 19:57 UTC (permalink / raw)
To: Brad Mouring, Andrew Lunn, Florian Fainelli; +Cc: netdev, linux-kernel
On 03/08/2018 10:41 PM, Sergei Shtylyov wrote:
>> If multiple phys share the same interrupt (e.g. a multi-phy chip),
>> the first device registered is the only one checked as phy_interrupt
>> will always return IRQ_HANDLED if the first phydev is not halted.
>> Move the interrupt check into phy_interrupt and, if it was not this
>> phydev, return IRQ_NONE to allow other devices on this irq a chance
>> to check if it was their interrupt.
>
> Hm, looking at kernel/irq/handle.c, all registered IRQ handlers are always
> called regardless of their results. Care to explain?
>
>> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
>> ---
>> drivers/net/phy/phy.c | 16 ++++++----------
>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index e3e29c2b028b..ff1aa815568f 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>> if (PHY_HALTED == phydev->state)
>> return IRQ_NONE; /* It can't be ours. */
>>
>> + if (phy_interrupt_is_valid(phydev)) {
>
> Always true in this context, no?
>
>> + if (phydev->drv->did_interrupt &&
>> + !phydev->drv->did_interrupt(phydev))
>
> I don't think we can do this in the interrupt context as this function *will*
> read from MDIO... I think that was the reason why IRQ handling is done in the
> thread context...
Ah, we're already in a thread context here! Forgot about it...
> [...]
MBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt
2018-03-08 19:41 ` Sergei Shtylyov
2018-03-08 19:57 ` Sergei Shtylyov
@ 2018-03-08 20:16 ` Brad Mouring
1 sibling, 0 replies; 11+ messages in thread
From: Brad Mouring @ 2018-03-08 20:16 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Andrew Lunn, Florian Fainelli, netdev, linux-kernel
Thanks for the feedback, Sergei.
On Thu, Mar 08, 2018 at 10:41:04PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 03/08/2018 01:50 AM, Brad Mouring wrote:
>
> > If multiple phys share the same interrupt (e.g. a multi-phy chip),
> > the first device registered is the only one checked as phy_interrupt
> > will always return IRQ_HANDLED if the first phydev is not halted.
> > Move the interrupt check into phy_interrupt and, if it was not this
> > phydev, return IRQ_NONE to allow other devices on this irq a chance
> > to check if it was their interrupt.
>
> Hm, looking at kernel/irq/handle.c, all registered IRQ handlers are always
> called regardless of their results. Care to explain?
In the phy interrupt handler case, the phy_interrupt function is
registered as the threaded secondary, and irq_default_primary_handler
is being used as the primary (which will turn around and wake the
threaded handler). It seems that we wake the thread_fns in order, and
the first to report back HANDLED stops us from waking the next.
> > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > ---
> > drivers/net/phy/phy.c | 16 ++++++----------
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index e3e29c2b028b..ff1aa815568f 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> > if (PHY_HALTED == phydev->state)
> > return IRQ_NONE; /* It can't be ours. */
> >
> > + if (phy_interrupt_is_valid(phydev)) {
>
> Always true in this context, no?
Yes, already noted.
> > + if (phydev->drv->did_interrupt &&
> > + !phydev->drv->did_interrupt(phydev))
>
> I don't think we can do this in the interrupt context as this function *will*
> read from MDIO... I think that was the reason why IRQ handling is done in the
> thread context...
phy_interrupt is the thread_fn here. We're not in interrupt context.
> > + return IRQ_NONE;
> > + }
> > +
> > phy_change(phydev);
> >
> > return IRQ_HANDLED;
> [...]
>
> MBR, Sergei
Thanks,
Brad
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt
2018-03-07 22:50 [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt Brad Mouring
2018-03-08 16:29 ` Andrew Lunn
2018-03-08 19:41 ` Sergei Shtylyov
@ 2018-03-08 20:25 ` Sergei Shtylyov
2018-03-08 20:31 ` [PATCH] net: phy: Tell caller result of phy_change() Brad Mouring
3 siblings, 0 replies; 11+ messages in thread
From: Sergei Shtylyov @ 2018-03-08 20:25 UTC (permalink / raw)
To: Brad Mouring, Andrew Lunn, Florian Fainelli; +Cc: netdev, linux-kernel
On 03/08/2018 01:50 AM, Brad Mouring wrote:
> If multiple phys share the same interrupt (e.g. a multi-phy chip),
> the first device registered is the only one checked as phy_interrupt
> will always return IRQ_HANDLED if the first phydev is not halted.
> Move the interrupt check into phy_interrupt and, if it was not this
> phydev, return IRQ_NONE to allow other devices on this irq a chance
> to check if it was their interrupt.
>
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> ---
> drivers/net/phy/phy.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e3e29c2b028b..ff1aa815568f 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> if (PHY_HALTED == phydev->state)
> return IRQ_NONE; /* It can't be ours. */
>
> + if (phy_interrupt_is_valid(phydev)) {
> + if (phydev->drv->did_interrupt &&
> + !phydev->drv->did_interrupt(phydev))
Forgot to mention: DaveM prefers that continuation lines start under the 1st
column after ( on the broken up line. The way you did it this line blends with
the next one (they shouldn't).
> + return IRQ_NONE;
> + }
> +
> phy_change(phydev);
>
> return IRQ_HANDLED;
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] net: phy: Tell caller result of phy_change()
2018-03-07 22:50 [PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt Brad Mouring
` (2 preceding siblings ...)
2018-03-08 20:25 ` Sergei Shtylyov
@ 2018-03-08 20:31 ` Brad Mouring
2018-03-08 20:47 ` Andrew Lunn
2018-03-08 22:23 ` [PATCH net v3] " Brad Mouring
3 siblings, 2 replies; 11+ messages in thread
From: Brad Mouring @ 2018-03-08 20:31 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli
Cc: netdev, linux-kernel, Sergei Shtylyov, Brad Mouring
In 664fcf123a30e (net: phy: Threaded interrupts allow some simplification)
the phy_interrupt system was changed to use a traditional threaded
interrupt scheme instead of a workqueue approach.
With this change, the phy status check moved into phy_change, which
did not report back to the caller whether or not the interrupt was
handled. This means that, in the case of a shared phy interrupt,
only the first phydev's interrupt registers are checked (since
phy_interrupt() would always return IRQ_HANDLED). This leads to
interrupt storms when it is a secondary device that's actually the
interrupt source.
Signed-off-by: Brad Mouring <brad.mouring@ni.com>
---
drivers/net/phy/phy.c | 13 +++++++------
include/linux/phy.h | 1 -
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..ebce9f2b2742 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -617,6 +617,8 @@ static void phy_error(struct phy_device *phydev)
phy_trigger_machine(phydev, false);
}
+static irqreturn_t phy_change(struct phy_device *phydev);
+
/**
* phy_interrupt - PHY interrupt handler
* @irq: interrupt line
@@ -632,9 +634,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
if (PHY_HALTED == phydev->state)
return IRQ_NONE; /* It can't be ours. */
- phy_change(phydev);
-
- return IRQ_HANDLED;
+ return phy_change(phydev);
}
/**
@@ -723,12 +723,12 @@ EXPORT_SYMBOL(phy_stop_interrupts);
* phy_change - Called by the phy_interrupt to handle PHY changes
* @phydev: phy_device struct that interrupted
*/
-void phy_change(struct phy_device *phydev)
+static irqreturn_t phy_change(struct phy_device *phydev)
{
if (phy_interrupt_is_valid(phydev)) {
if (phydev->drv->did_interrupt &&
!phydev->drv->did_interrupt(phydev))
- return;
+ return IRQ_NONE;
if (phydev->state == PHY_HALTED)
if (phy_disable_interrupts(phydev))
@@ -745,10 +745,11 @@ void phy_change(struct phy_device *phydev)
if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
goto phy_err;
- return;
+ return IRQ_HANDLED;
phy_err:
phy_error(phydev);
+ return IRQ_NONE;
}
/**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a0c3e53e7c2..2e55838655b9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1011,7 +1011,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
int phy_drivers_register(struct phy_driver *new_driver, int n,
struct module *owner);
void phy_state_machine(struct work_struct *work);
-void phy_change(struct phy_device *phydev);
void phy_change_work(struct work_struct *work);
void phy_mac_interrupt(struct phy_device *phydev);
void phy_start_machine(struct phy_device *phydev);
--
2.16.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] net: phy: Tell caller result of phy_change()
2018-03-08 20:31 ` [PATCH] net: phy: Tell caller result of phy_change() Brad Mouring
@ 2018-03-08 20:47 ` Andrew Lunn
2018-03-08 22:23 ` [PATCH net v3] " Brad Mouring
1 sibling, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2018-03-08 20:47 UTC (permalink / raw)
To: Brad Mouring; +Cc: Florian Fainelli, netdev, linux-kernel, Sergei Shtylyov
> +++ b/drivers/net/phy/phy.c
> @@ -617,6 +617,8 @@ static void phy_error(struct phy_device *phydev)
> phy_trigger_machine(phydev, false);
> }
>
> +static irqreturn_t phy_change(struct phy_device *phydev);
> +
Hi Brad
Rather than add a forward declaration, please move phy_change().
Thanks
Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net v3] net: phy: Tell caller result of phy_change()
2018-03-08 20:31 ` [PATCH] net: phy: Tell caller result of phy_change() Brad Mouring
2018-03-08 20:47 ` Andrew Lunn
@ 2018-03-08 22:23 ` Brad Mouring
2018-03-12 14:34 ` David Miller
1 sibling, 1 reply; 11+ messages in thread
From: Brad Mouring @ 2018-03-08 22:23 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli
Cc: netdev, linux-kernel, Sergei Shtylyov, Brad Mouring
In 664fcf123a30e (net: phy: Threaded interrupts allow some simplification)
the phy_interrupt system was changed to use a traditional threaded
interrupt scheme instead of a workqueue approach.
With this change, the phy status check moved into phy_change, which
did not report back to the caller whether or not the interrupt was
handled. This means that, in the case of a shared phy interrupt,
only the first phydev's interrupt registers are checked (since
phy_interrupt() would always return IRQ_HANDLED). This leads to
interrupt storms when it is a secondary device that's actually the
interrupt source.
Signed-off-by: Brad Mouring <brad.mouring@ni.com>
---
drivers/net/phy/phy.c | 145 +++++++++++++++++++++++++-------------------------
include/linux/phy.h | 1 -
2 files changed, 72 insertions(+), 74 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..dfff2cff7da9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -617,6 +617,77 @@ static void phy_error(struct phy_device *phydev)
phy_trigger_machine(phydev, false);
}
+/**
+ * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
+ * @phydev: target phy_device struct
+ */
+static int phy_disable_interrupts(struct phy_device *phydev)
+{
+ int err;
+
+ /* Disable PHY interrupts */
+ err = phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
+ if (err)
+ goto phy_err;
+
+ /* Clear the interrupt */
+ err = phy_clear_interrupt(phydev);
+ if (err)
+ goto phy_err;
+
+ return 0;
+
+phy_err:
+ phy_error(phydev);
+
+ return err;
+}
+
+/**
+ * phy_change - Called by the phy_interrupt to handle PHY changes
+ * @phydev: phy_device struct that interrupted
+ */
+static irqreturn_t phy_change(struct phy_device *phydev)
+{
+ if (phy_interrupt_is_valid(phydev)) {
+ if (phydev->drv->did_interrupt &&
+ !phydev->drv->did_interrupt(phydev))
+ return IRQ_NONE;
+
+ if (phydev->state == PHY_HALTED)
+ if (phy_disable_interrupts(phydev))
+ goto phy_err;
+ }
+
+ mutex_lock(&phydev->lock);
+ if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
+ phydev->state = PHY_CHANGELINK;
+ mutex_unlock(&phydev->lock);
+
+ /* reschedule state queue work to run as soon as possible */
+ phy_trigger_machine(phydev, true);
+
+ if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
+ goto phy_err;
+ return IRQ_HANDLED;
+
+phy_err:
+ phy_error(phydev);
+ return IRQ_NONE;
+}
+
+/**
+ * phy_change_work - Scheduled by the phy_mac_interrupt to handle PHY changes
+ * @work: work_struct that describes the work to be done
+ */
+void phy_change_work(struct work_struct *work)
+{
+ struct phy_device *phydev =
+ container_of(work, struct phy_device, phy_queue);
+
+ phy_change(phydev);
+}
+
/**
* phy_interrupt - PHY interrupt handler
* @irq: interrupt line
@@ -632,9 +703,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
if (PHY_HALTED == phydev->state)
return IRQ_NONE; /* It can't be ours. */
- phy_change(phydev);
-
- return IRQ_HANDLED;
+ return phy_change(phydev);
}
/**
@@ -651,32 +720,6 @@ static int phy_enable_interrupts(struct phy_device *phydev)
return phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
}
-/**
- * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
- * @phydev: target phy_device struct
- */
-static int phy_disable_interrupts(struct phy_device *phydev)
-{
- int err;
-
- /* Disable PHY interrupts */
- err = phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
- if (err)
- goto phy_err;
-
- /* Clear the interrupt */
- err = phy_clear_interrupt(phydev);
- if (err)
- goto phy_err;
-
- return 0;
-
-phy_err:
- phy_error(phydev);
-
- return err;
-}
-
/**
* phy_start_interrupts - request and enable interrupts for a PHY device
* @phydev: target phy_device struct
@@ -719,50 +762,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_stop_interrupts);
-/**
- * phy_change - Called by the phy_interrupt to handle PHY changes
- * @phydev: phy_device struct that interrupted
- */
-void phy_change(struct phy_device *phydev)
-{
- if (phy_interrupt_is_valid(phydev)) {
- if (phydev->drv->did_interrupt &&
- !phydev->drv->did_interrupt(phydev))
- return;
-
- if (phydev->state == PHY_HALTED)
- if (phy_disable_interrupts(phydev))
- goto phy_err;
- }
-
- mutex_lock(&phydev->lock);
- if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
- phydev->state = PHY_CHANGELINK;
- mutex_unlock(&phydev->lock);
-
- /* reschedule state queue work to run as soon as possible */
- phy_trigger_machine(phydev, true);
-
- if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
- goto phy_err;
- return;
-
-phy_err:
- phy_error(phydev);
-}
-
-/**
- * phy_change_work - Scheduled by the phy_mac_interrupt to handle PHY changes
- * @work: work_struct that describes the work to be done
- */
-void phy_change_work(struct work_struct *work)
-{
- struct phy_device *phydev =
- container_of(work, struct phy_device, phy_queue);
-
- phy_change(phydev);
-}
-
/**
* phy_stop - Bring down the PHY link, and stop checking the status
* @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a0c3e53e7c2..2e55838655b9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1011,7 +1011,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
int phy_drivers_register(struct phy_driver *new_driver, int n,
struct module *owner);
void phy_state_machine(struct work_struct *work);
-void phy_change(struct phy_device *phydev);
void phy_change_work(struct work_struct *work);
void phy_mac_interrupt(struct phy_device *phydev);
void phy_start_machine(struct phy_device *phydev);
--
2.16.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net v3] net: phy: Tell caller result of phy_change()
2018-03-08 22:23 ` [PATCH net v3] " Brad Mouring
@ 2018-03-12 14:34 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2018-03-12 14:34 UTC (permalink / raw)
To: brad.mouring; +Cc: andrew, f.fainelli, netdev, linux-kernel, sergei.shtylyov
From: Brad Mouring <brad.mouring@ni.com>
Date: Thu, 8 Mar 2018 16:23:03 -0600
> In 664fcf123a30e (net: phy: Threaded interrupts allow some simplification)
> the phy_interrupt system was changed to use a traditional threaded
> interrupt scheme instead of a workqueue approach.
>
> With this change, the phy status check moved into phy_change, which
> did not report back to the caller whether or not the interrupt was
> handled. This means that, in the case of a shared phy interrupt,
> only the first phydev's interrupt registers are checked (since
> phy_interrupt() would always return IRQ_HANDLED). This leads to
> interrupt storms when it is a secondary device that's actually the
> interrupt source.
>
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread