On 26.02.2020 12:12, Michael Walle wrote:

Am 2020-02-26 08:27, schrieb Heiner Kallweit:

quoted

On 26.02.2020 00:08, Michael Walle wrote:

quoted

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/ (local)

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