| Message ID | 1384376870-7810-1-git-send-email-zonque@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Daniel Mack <zonque@gmail.com> Date: Wed, 13 Nov 2013 22:07:49 +0100 > Allow phy drivers to take action when the core does its link adjustment. > No change for drivers that do not implement this callback. > > Signed-off-by: Daniel Mack <zonque@gmail.com> So you're using this to reset the entire PHY via the reset bit in the BMCR register when the link goes down. But this is going to break things. If the phy library previously programmed a non-autonegotiated static link configuration into the BMCR register, your reset is going to undo that. Now the configuration phylib thinks the chip has and the one it acutally does is out of sync. I'm not applying these patches, sorry. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/14/2013 10:32 PM, David Miller wrote: > From: Daniel Mack <zonque@gmail.com> > Date: Wed, 13 Nov 2013 22:07:49 +0100 > >> Allow phy drivers to take action when the core does its link adjustment. >> No change for drivers that do not implement this callback. >> >> Signed-off-by: Daniel Mack <zonque@gmail.com> > > So you're using this to reset the entire PHY via the reset bit in the > BMCR register when the link goes down. > > But this is going to break things. > > If the phy library previously programmed a non-autonegotiated static > link configuration into the BMCR register, your reset is going to > undo that. > > Now the configuration phylib thinks the chip has and the one it > acutally does is out of sync. You're right, thanks for the review. Let me just state that I'm really unhappy about that approach as well. However, I currently see no better way than resetting the PHY every time the link goes down, as we have no way of telling whether the chip has entered its locked-up state which it seemingly does arbitrarily when the link changes. This is really not nice, but the only thing that avoided the effect in our tests. That means that we either have to tell the PHY core about what we did, or manually preserve the registers contents across the reset. I'll have another look next week. Any pointers appreciated :) Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 36c6994..240e33f 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -748,6 +748,9 @@ void phy_state_machine(struct work_struct *work) if (phydev->adjust_state) phydev->adjust_state(phydev->attached_dev); + if (phydev->drv->adjust_state) + phydev->drv->adjust_state(phydev); + switch(phydev->state) { case PHY_DOWN: case PHY_STARTING: diff --git a/include/linux/phy.h b/include/linux/phy.h index 64ab823..85826eb 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -467,6 +467,8 @@ struct phy_driver { /* See set_wol, but for checking whether Wake on LAN is enabled. */ void (*get_wol)(struct phy_device *dev, struct ethtool_wolinfo *wol); + void (*adjust_state)(struct phy_device *dev); + struct device_driver driver; }; #define to_phy_driver(d) container_of(d, struct phy_driver, driver)
Allow phy drivers to take action when the core does its link adjustment. No change for drivers that do not implement this callback. Signed-off-by: Daniel Mack <zonque@gmail.com> --- drivers/net/phy/phy.c | 3 +++ include/linux/phy.h | 2 ++ 2 files changed, 5 insertions(+)