Re: [RFC PATCH 1/2] net: phy: let the driver register its own IRQ handler

[Heiner Kallweit <hkallweit1@gmail.com>]

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