[net-next,6/7] net: phy: smsc: Cache interrupt mask

Commit Message

Lukas Wunner April 27, 2022, 5:48 a.m. UTC

Cache the interrupt mask to avoid re-reading it from the PHY upon every
interrupt.  The PHY may be located on a USB device, so the additional
read may unnecessarily increase interrupt overhead and latency.

Signed-off-by: Lukas Wunner <[e-mail]>
---
 drivers/net/phy/smsc.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Comments

Andrew Lunn April 27, 2022, 12:14 p.m. UTC | #1

On Wed, Apr 27, 2022 at 07:48:06AM +0200, Lukas Wunner wrote:
> Cache the interrupt mask to avoid re-reading it from the PHY upon every
> interrupt.  The PHY may be located on a USB device, so the additional
> read may unnecessarily increase interrupt overhead and latency.

I don't think your justification is valid. The MDIO bus is clocked at
2.5MHz. So even if you are using USB 1.1 at 12MHz, the USB overheads
are not particularly large. At 480Mbps they are pretty insignificant.

In general, we consider PHYs as slow devices, they take over 1 second
to negotiate a link and declare it up. So we don't do this sort of
micro optimization.

What i think is relevant here is that you could have an interrupt
storm going on because you don't mask interrupts? It is not a true
storm, due to the way USB works, more of a light shower. Do you have
any statistics to show this code actually reduces the amount of rain
in a significant way?

	Andrew

Lukas Wunner April 27, 2022, 12:51 p.m. UTC | #2

On Wed, Apr 27, 2022 at 02:14:22PM +0200, Andrew Lunn wrote:
> On Wed, Apr 27, 2022 at 07:48:06AM +0200, Lukas Wunner wrote:
> > Cache the interrupt mask to avoid re-reading it from the PHY upon every
> > interrupt.  The PHY may be located on a USB device, so the additional
> > read may unnecessarily increase interrupt overhead and latency.
> 
> I don't think your justification is valid. The MDIO bus is clocked at
> 2.5MHz. So even if you are using USB 1.1 at 12MHz, the USB overheads
> are not particularly large. At 480Mbps they are pretty insignificant.
> 
> In general, we consider PHYs as slow devices, they take over 1 second
> to negotiate a link and declare it up. So we don't do this sort of
> micro optimization.
> 
> What i think is relevant here is that you could have an interrupt
> storm going on because you don't mask interrupts? It is not a true
> storm, due to the way USB works, more of a light shower. Do you have
> any statistics to show this code actually reduces the amount of rain
> in a significant way?

TBH the primary motivation for this change is that it simplifies the
succeeding commit ("Cope with hot-removal in interrupt handler").

Additionally it seemed silly to me to re-read the interrupt mask
every time for no reason at all.  To test and debug this series
I logged every MDIO read/write and these nonsensical transactions
are very visible and very annoying in the log output.

So yeah, maybe the latency argument isn't very strong, but there
are other arguments which I didn't deem necessary mentioning in
the commit message as they seemed somewhat egotistical. :)

Thanks,

Lukas

Patch

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index d8cac02a79b9..a521d48b22a7 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -44,6 +44,7 @@  static struct smsc_hw_stat smsc_hw_stats[] = {
 };
 
 struct smsc_phy_priv {
+	u16 intmask;
 	bool energy_enable;
 	struct clk *refclk;
 };
@@ -58,7 +59,6 @@  static int smsc_phy_ack_interrupt(struct phy_device *phydev)
 static int smsc_phy_config_intr(struct phy_device *phydev)
 {
 	struct smsc_phy_priv *priv = phydev->priv;
-	u16 intmask = 0;
 	int rc;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
@@ -66,12 +66,15 @@  static int smsc_phy_config_intr(struct phy_device *phydev)
 		if (rc)
 			return rc;
 
-		intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
+		priv->intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
 		if (priv->energy_enable)
-			intmask |= MII_LAN83C185_ISF_INT7;
-		rc = phy_write(phydev, MII_LAN83C185_IM, intmask);
+			priv->intmask |= MII_LAN83C185_ISF_INT7;
+
+		rc = phy_write(phydev, MII_LAN83C185_IM, priv->intmask);
 	} else {
-		rc = phy_write(phydev, MII_LAN83C185_IM, intmask);
+		priv->intmask = 0;
+
+		rc = phy_write(phydev, MII_LAN83C185_IM, 0);
 		if (rc)
 			return rc;
 
@@ -83,13 +86,8 @@  static int smsc_phy_config_intr(struct phy_device *phydev)
 
 static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
 {
-	int irq_status, irq_enabled;
-
-	irq_enabled = phy_read(phydev, MII_LAN83C185_IM);
-	if (irq_enabled < 0) {
-		phy_error(phydev);
-		return IRQ_NONE;
-	}
+	struct smsc_phy_priv *priv = phydev->priv;
+	int irq_status;
 
 	irq_status = phy_read(phydev, MII_LAN83C185_ISF);
 	if (irq_status < 0) {
@@ -97,7 +95,7 @@  static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
 		return IRQ_NONE;
 	}
 
-	if (!(irq_status & irq_enabled))
+	if (!(irq_status & priv->intmask))
 		return IRQ_NONE;
 
 	phy_trigger_machine(phydev);