| Message ID | 20180307225042.2205-1-brad.mouring@ni.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: Move interrupt check from phy_check to phy_interrupt | expand |
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
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
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
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
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
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
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)) + 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; - } - mutex_lock(&phydev->lock); if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state)) phydev->state = PHY_CHANGELINK;
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(-)