| Message ID | 1498211042-240816-2-git-send-email-linyunsheng@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> +int phy_loopback(struct phy_device *phydev, bool enable) > +{ > + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); > + int ret = 0; > + > + if (enable && phydev->loopback_enabled) > + return -EBUSY; > + > + if (!enable && !phydev->loopback_enabled) > + return -EINVAL; > + > + if (phydev->drv && phydrv->set_loopback) > + ret = phydrv->set_loopback(phydev, enable); else ret = -EOPNOTSUPP; > + > + if (ret) > + return ret; > + > + phydev->loopback_enabled = enable; > + > + return 0; > +} > +EXPORT_SYMBOL(phy_loopback); One of the comments we made of the PHY code in the hns driver is that its locking is completely broken. You have made the same error here. The core needs to hold the mutex while calling into the PHY driver. Andrew
Hi, Andrew On 2017/6/24 11:12, Andrew Lunn wrote: >> +int phy_loopback(struct phy_device *phydev, bool enable) >> +{ >> + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); >> + int ret = 0; >> + >> + if (enable && phydev->loopback_enabled) >> + return -EBUSY; >> + >> + if (!enable && !phydev->loopback_enabled) >> + return -EINVAL; >> + >> + if (phydev->drv && phydrv->set_loopback) >> + ret = phydrv->set_loopback(phydev, enable); > > else > ret = -EOPNOTSUPP; > >> + >> + if (ret) >> + return ret; >> + >> + phydev->loopback_enabled = enable; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(phy_loopback); > > One of the comments we made of the PHY code in the hns driver is that > its locking is completely broken. You have made the same error > here. The core needs to hold the mutex while calling into the PHY > driver. Do you mean hns_nic_config_phy_loopback need to hold the mutex while calling phy_loopback? and other place that calling phy_* function? Best Regards Yunsheng Lin
On 2017/6/24 11:40, Yunsheng Lin wrote: > Hi, Andrew > > On 2017/6/24 11:12, Andrew Lunn wrote: >>> +int phy_loopback(struct phy_device *phydev, bool enable) >>> +{ >>> + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); >>> + int ret = 0; >>> + >>> + if (enable && phydev->loopback_enabled) >>> + return -EBUSY; >>> + >>> + if (!enable && !phydev->loopback_enabled) >>> + return -EINVAL; >>> + >>> + if (phydev->drv && phydrv->set_loopback) >>> + ret = phydrv->set_loopback(phydev, enable); >> >> else >> ret = -EOPNOTSUPP; >> >>> + >>> + if (ret) >>> + return ret; >>> + >>> + phydev->loopback_enabled = enable; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(phy_loopback); >> >> One of the comments we made of the PHY code in the hns driver is that >> its locking is completely broken. You have made the same error >> here. The core needs to hold the mutex while calling into the PHY >> driver. > Do you mean hns_nic_config_phy_loopback need to hold the mutex while > calling phy_loopback? and other place that calling phy_* function? I took some time looking into how to take mutex in phy core, here is what I find: phy_resume call phydrv->resume without take mutex. if phy driver implement resume function, for example marvell_resume, then static int marvell_resume(struct phy_device *phydev) { int err; /* Resume the fiber mode first */ if (!(phydev->supported & SUPPORTED_FIBRE)) { err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER); if (err < 0) goto error; /* With the page set, use the generic resume */ err = genphy_resume(phydev); if (err < 0) goto error; /* Then, the copper link */ err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER); if (err < 0) goto error; } the code above executes without holding a mutex expect genphy_resume /* With the page set, use the generic resume */ return genphy_resume(phydev); error: phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER); return err; } So I think the correct way to hold a lock is in phy_* function, not in genphy_*, and current genphy_resume and phy_resume is broken in this way. Please let me know if I misunderstand the mutex taking in phy core. Best Regards Yunsheng Lin
> phy_resume call phydrv->resume without take mutex. > if phy driver implement resume function, for example marvell_resume, then > > static int marvell_resume(struct phy_device *phydev) > { > int err; > > /* Resume the fiber mode first */ > if (!(phydev->supported & SUPPORTED_FIBRE)) { > err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER); > if (err < 0) > goto error; > > /* With the page set, use the generic resume */ > err = genphy_resume(phydev); > if (err < 0) > goto error; > > /* Then, the copper link */ > err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER); > if (err < 0) > goto error; > } > > the code above executes without holding a mutex expect genphy_resume > > /* With the page set, use the generic resume */ > return genphy_resume(phydev); > > error: > phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER); > return err; > } > > So I think the correct way to hold a lock is in phy_* function, not in genphy_*, > and current genphy_resume and phy_resume is broken in this way. Suspend and resume are a bit odd. As you found out, everywhere else, the core takes the mutex before calling into the driver function. The general rule of thumb, is that many driver writers don't understand locking. So it is better to do the locking in the core, which is generally written by people who do understand locking. And core code gets used more, so bugs tend to be found. Andrew
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 57297ba..01a1586 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -2094,6 +2094,7 @@ static int m88e1510_probe(struct phy_device *phydev) .get_sset_count = marvell_get_sset_count, .get_strings = marvell_get_strings, .get_stats = marvell_get_stats, + .set_loopback = genphy_loopback, }, { .phy_id = MARVELL_PHY_ID_88E1540, diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 1219eea..1e8f800 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1123,6 +1123,29 @@ int phy_resume(struct phy_device *phydev) } EXPORT_SYMBOL(phy_resume); +int phy_loopback(struct phy_device *phydev, bool enable) +{ + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); + int ret = 0; + + if (enable && phydev->loopback_enabled) + return -EBUSY; + + if (!enable && !phydev->loopback_enabled) + return -EINVAL; + + if (phydev->drv && phydrv->set_loopback) + ret = phydrv->set_loopback(phydev, enable); + + if (ret) + return ret; + + phydev->loopback_enabled = enable; + + return 0; +} +EXPORT_SYMBOL(phy_loopback); + /* Generic PHY support and helper functions */ /** @@ -1628,6 +1651,23 @@ static int gen10g_resume(struct phy_device *phydev) return 0; } +int genphy_loopback(struct phy_device *phydev, bool enable) +{ + int value; + + value = phy_read(phydev, MII_BMCR); + if (value < 0) + return value; + + if (enable) + value |= BMCR_LOOPBACK; + else + value &= ~BMCR_LOOPBACK; + + return phy_write(phydev, MII_BMCR, value); +} +EXPORT_SYMBOL(genphy_loopback); + static int __set_phy_supported(struct phy_device *phydev, u32 max_speed) { /* The default values for phydev->supported are provided by the PHY @@ -1874,6 +1914,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) .read_status = genphy_read_status, .suspend = genphy_suspend, .resume = genphy_resume, + .set_loopback = genphy_loopback, }, { .phy_id = 0xffffffff, .phy_id_mask = 0xffffffff, diff --git a/include/linux/phy.h b/include/linux/phy.h index e76e4ad..49c903dc 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -364,6 +364,7 @@ struct phy_c45_device_ids { * is_pseudo_fixed_link: Set to true if this phy is an Ethernet switch, etc. * has_fixups: Set to true if this phy has fixups/quirks. * suspended: Set to true if this phy has been suspended successfully. + * loopback_enabled: Set true if this phy has been loopbacked successfully. * state: state of the PHY for management purposes * dev_flags: Device-specific flags used by the PHY driver. * link_timeout: The number of timer firings to wait before the @@ -400,6 +401,7 @@ struct phy_device { bool is_pseudo_fixed_link; bool has_fixups; bool suspended; + bool loopback_enabled; enum phy_state state; @@ -639,6 +641,7 @@ struct phy_driver { int (*set_tunable)(struct phy_device *dev, struct ethtool_tunable *tuna, const void *data); + int (*set_loopback)(struct phy_device *dev, bool enable); }; #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \ struct phy_driver, mdiodrv) @@ -774,6 +777,7 @@ static inline void phy_device_free(struct phy_device *phydev) { } int phy_init_hw(struct phy_device *phydev); int phy_suspend(struct phy_device *phydev); int phy_resume(struct phy_device *phydev); +int phy_loopback(struct phy_device *phydev, bool enable); struct phy_device *phy_attach(struct net_device *dev, const char *bus_id, phy_interface_t interface); struct phy_device *phy_find_first(struct mii_bus *bus); @@ -825,6 +829,7 @@ void phy_attached_print(struct phy_device *phydev, const char *fmt, ...) int genphy_read_status(struct phy_device *phydev); int genphy_suspend(struct phy_device *phydev); int genphy_resume(struct phy_device *phydev); +int genphy_loopback(struct phy_device *phydev, bool enable); int genphy_soft_reset(struct phy_device *phydev); static inline int genphy_no_soft_reset(struct phy_device *phydev) {
This patch add set_loopback in phy_driver, which is used by Mac driver to enable or disable a phy. it also add a generic genphy_loopback function, which use BMCR loopback bit to enable or disable a phy. Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> --- drivers/net/phy/marvell.c | 1 + drivers/net/phy/phy_device.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 5 +++++ 3 files changed, 47 insertions(+)