Messages in this thread Patch in this message | | | Subject | Re: [RFC PATCH 1/2] net: phy: let the driver register its own IRQ handler | From | Heiner Kallweit <> | Date | Wed, 26 Feb 2020 22:17:00 +0100 |
| |
On 26.02.2020 12:12, Michael Walle wrote: > Am 2020-02-26 08:27, schrieb Heiner Kallweit: >> On 26.02.2020 00:08, Michael Walle wrote: >>> There are more and more PHY drivers which has more than just the PHY >>> link change interrupts. For example, temperature thresholds or PTP >>> interrupts. >>> >>> At the moment it is not possible to correctly handle interrupts for PHYs >>> which has a clear-on-read interrupt status register. It is also likely >>> that the current approach of the phylib isn't working for all PHYs out >>> there. >>> >>> Therefore, this patch let the PHY driver register its own interrupt >>> handler. To notify the phylib about a link change, the interrupt handler >>> has to call the new function phy_drv_interrupt(). >>> >> >> We have phy_driver callback handle_interrupt for custom interrupt >> handlers. Any specific reason why you can't use it for your purposes? > > Yes, as mentioned above this wont work for PHYs which has a clear-on-read > status register, because you may loose interrupts between handle_interrupt() > and phy_clear_interrupt(). > > See also > > https://lore.kernel.org/netdev/bd47f8e1ebc04fa98856ed8d89b91419@walle.cc/ > > And esp. Russell reply. I tried using handle_interrupt() but it won't work > unless you make the ack_interrupt a NOOP. but even then it won't work because > the ack_interrupt is also used during setup etc. > Right, now I remember .. So far we have only one user of the handle_interrupt callback. Following a proposal from the quoted discussion, can you base your patch on the following and check? Note: Even though you implement handle_interrupt, you still have to implement ack_interrupt too.
--- drivers/net/phy/mscc.c | 3 ++- drivers/net/phy/phy.c | 4 ++-- include/linux/phy.h | 4 +++- 3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c index 937ac7da2..20b9d3ef5 100644 --- a/drivers/net/phy/mscc.c +++ b/drivers/net/phy/mscc.c @@ -2868,7 +2868,8 @@ static int vsc8584_handle_interrupt(struct phy_device *phydev) #endif phy_mac_interrupt(phydev); - return 0; + + return vsc85xx_ack_interrupt(phydev); } static int vsc85xx_config_init(struct phy_device *phydev) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index d76e038cf..de52f0e82 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -725,10 +725,10 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat) } else { /* reschedule state queue work to run as soon as possible */ phy_trigger_machine(phydev); + if (phy_clear_interrupt(phydev)) + goto phy_err; } - if (phy_clear_interrupt(phydev)) - goto phy_err; return IRQ_HANDLED; phy_err: diff --git a/include/linux/phy.h b/include/linux/phy.h index 80f8b2158..9e2895ee4 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -560,7 +560,9 @@ struct phy_driver { */ int (*did_interrupt)(struct phy_device *phydev); - /* Override default interrupt handling */ + /* Override default interrupt handling. Handler has to ensure + * that interrupt is ack'ed. + */ int (*handle_interrupt)(struct phy_device *phydev); /* Clears up any memory if needed */ -- 2.25.1
| |