Thread (5 messages) 5 messages, 3 authors, 2023-03-09

Re: [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend

From: Andrew Lunn <andrew@lunn.ch>
Date: 2023-02-28 15:05:40

+static int dp83867_suspend(struct phy_device *phydev)
+{
+	/* Disable PHY Interrupts */
+	if (phy_interrupt_is_valid(phydev)) {
+		phydev->interrupts = PHY_INTERRUPT_DISABLED;
+		if (phydev->drv->config_intr)
+			phydev->drv->config_intr(phydev);
It seems odd going via phydev->drv inside the driver to call functions
which are also inside the driver. Why do you not directly call dp83867_config_intr()?
+static int dp83867_resume(struct phy_device *phydev)
+{
+	genphy_resume(phydev);
+
+	/* Enable PHY Interrupts */
+	if (phy_interrupt_is_valid(phydev)) {
+		phydev->interrupts = PHY_INTERRUPT_ENABLED;
+		if (phydev->drv->config_intr)
+			phydev->drv->config_intr(phydev);
+	}
Is there a race here? Say the PHY is in a fixed mode, not
autoneg. Could it be, that as soon as you clear the power down bit in
genphy_resume() it signals a link up interrupt? dp83867_config_intr()
then acknowledged and clears that interrupt, before enabling the
interrupt, so the link up event never gets passed to phylib? Maybe the
order needs reversing here?

	   Andrew
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help