Re: [PATCH net] net: phy: Restore phy_resume() locking assumption
> > +int phy_resume(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + mutex_lock(>lock);
> > + ret = phy_resume(phydev);
>
> phy_resume -> __phy_resume?
Ah, where did i put the brown paper bag :-(
Thanks
Andrew
Re: [PATCH net] net: phy: Restore phy_resume() locking assumption
Hi, Andrew
On 2018/2/26 7:04, Lunn wrote:
> commit f5e64032a799 ("net: phy: fix resume handling") changes the
> locking semantics for phy_resume() such that the caller now needs to
> hold the phy mutex. Not all call sites were adopted to this new
> semantic, resulting in warnings from the added
> WARN_ON(!mutex_is_locked(>lock)). Rather than change the
> semantics, add a __phy_resume() and restore the old behavior of
> phy_resume().
>
> Reported-by: Heiner Kallweit
> Fixes: f5e64032a799 ("net: phy: fix resume handling")
> Signed-off-by: Andrew Lunn
> ---
> drivers/net/phy/phy.c| 2 +-
> drivers/net/phy/phy_device.c | 18 +-
> include/linux/phy.h | 1 +
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e3e29c2b028b..a6f924fee584 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -819,7 +819,7 @@ void phy_start(struct phy_device *phydev)
> break;
> case PHY_HALTED:
> /* if phy was suspended, bring the physical link up again */
> - phy_resume(phydev);
> + __phy_resume(phydev);
>
> /* make sure interrupts are re-enabled for the PHY */
> if (phy_interrupt_is_valid(phydev)) {
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d39ae77707ef..4cfb3851ed1f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -135,9 +135,7 @@ static int mdio_bus_phy_resume(struct device *dev)
> if (!mdio_bus_phy_may_suspend(phydev))
> goto no_resume;
>
> - mutex_lock(>lock);
> ret = phy_resume(phydev);
> - mutex_unlock(>lock);
> if (ret < 0)
> return ret;
>
> @@ -1041,9 +1039,7 @@ int phy_attach_direct(struct net_device *dev, struct
> phy_device *phydev,
> if (err)
> goto error;
>
> - mutex_lock(>lock);
> phy_resume(phydev);
> - mutex_unlock(>lock);
> phy_led_triggers_register(phydev);
>
> return err;
> @@ -1172,7 +1168,7 @@ int phy_suspend(struct phy_device *phydev)
> }
> EXPORT_SYMBOL(phy_suspend);
>
> -int phy_resume(struct phy_device *phydev)
> +int __phy_resume(struct phy_device *phydev)
> {
> struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
> int ret = 0;
> @@ -1189,6 +1185,18 @@ int phy_resume(struct phy_device *phydev)
>
> return ret;
> }
> +EXPORT_SYMBOL(__phy_resume);
> +
> +int phy_resume(struct phy_device *phydev)
> +{
> + int ret;
> +
> + mutex_lock(>lock);
> + ret = phy_resume(phydev);
phy_resume -> __phy_resume?
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> EXPORT_SYMBOL(phy_resume);
>
> int phy_loopback(struct phy_device *phydev, bool enable)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 5a0c3e53e7c2..d7069539f351 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -924,6 +924,7 @@ void phy_device_remove(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_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);
>
[PATCH net] net: phy: Restore phy_resume() locking assumption
commit f5e64032a799 ("net: phy: fix resume handling") changes the
locking semantics for phy_resume() such that the caller now needs to
hold the phy mutex. Not all call sites were adopted to this new
semantic, resulting in warnings from the added
WARN_ON(!mutex_is_locked(>lock)). Rather than change the
semantics, add a __phy_resume() and restore the old behavior of
phy_resume().
Reported-by: Heiner Kallweit
Fixes: f5e64032a799 ("net: phy: fix resume handling")
Signed-off-by: Andrew Lunn
---
drivers/net/phy/phy.c| 2 +-
drivers/net/phy/phy_device.c | 18 +-
include/linux/phy.h | 1 +
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..a6f924fee584 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -819,7 +819,7 @@ void phy_start(struct phy_device *phydev)
break;
case PHY_HALTED:
/* if phy was suspended, bring the physical link up again */
- phy_resume(phydev);
+ __phy_resume(phydev);
/* make sure interrupts are re-enabled for the PHY */
if (phy_interrupt_is_valid(phydev)) {
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d39ae77707ef..4cfb3851ed1f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -135,9 +135,7 @@ static int mdio_bus_phy_resume(struct device *dev)
if (!mdio_bus_phy_may_suspend(phydev))
goto no_resume;
- mutex_lock(>lock);
ret = phy_resume(phydev);
- mutex_unlock(>lock);
if (ret < 0)
return ret;
@@ -1041,9 +1039,7 @@ int phy_attach_direct(struct net_device *dev, struct
phy_device *phydev,
if (err)
goto error;
- mutex_lock(>lock);
phy_resume(phydev);
- mutex_unlock(>lock);
phy_led_triggers_register(phydev);
return err;
@@ -1172,7 +1168,7 @@ int phy_suspend(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_suspend);
-int phy_resume(struct phy_device *phydev)
+int __phy_resume(struct phy_device *phydev)
{
struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
int ret = 0;
@@ -1189,6 +1185,18 @@ int phy_resume(struct phy_device *phydev)
return ret;
}
+EXPORT_SYMBOL(__phy_resume);
+
+int phy_resume(struct phy_device *phydev)
+{
+ int ret;
+
+ mutex_lock(>lock);
+ ret = phy_resume(phydev);
+ mutex_unlock(>lock);
+
+ return ret;
+}
EXPORT_SYMBOL(phy_resume);
int phy_loopback(struct phy_device *phydev, bool enable)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a0c3e53e7c2..d7069539f351 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -924,6 +924,7 @@ void phy_device_remove(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_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);
--
2.16.2
