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