[PATCH net v2 0/3] net: phy: Unbind/bind fixes

Hi all,

This patch series addresses the inability to safely unbind and bind
PHY drivers by making the appropriate checks throught PHYLIB where we
may be directly responding to user-space queries, as well as from within
the kernel state machine.

The second patch makes the unbind -> bind working by taking care of the
PHY state machine state.

Changes in v2:

- fixed net: phy: Fix lack of reference count on PHY driver against
  the Generic PHY driver which is special

Florian Fainelli (3):
  net: phy: Fix PHY module checks
  net: phy: Check phydev->drv
  net: phy: Fix PHY driver bind and unbind events

 drivers/net/phy/phy.c        | 26 ++++++++++++++++++++++----
 drivers/net/phy/phy_device.c | 35 ++++++++++++++++++++++++++++++-----
 include/linux/phy.h          |  3 +++
 3 files changed, 55 insertions(+), 9 deletions(-)

-- 
2.9.3

The Generic PHY drivers gets assigned after we checked that the current PHY
driver is NULL, so we need to check a few things before we can safely
derference d->driver. Update phy_attach_direct() and phy_detach() accordingly
to be resilient to these cases.

Even though the Generic PHY driver defaults to phy_probe() which can hardly
fail at the moment, let's fix the label so we don't call phy_detach() on a
network device we have not attached yet.

Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0d8f4d3847f6..bde240bf8d7b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -920,7 +920,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		return -EIO;
 	}
 
-	if (!try_module_get(d->driver->owner)) {
+	if (d->driver && !try_module_get(d->driver->owner)) {
 		dev_err(&dev->dev, "failed to get the device driver module\n");
 		return -EIO;
 	}

@@ -943,7 +943,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			err = device_bind_driver(d);
 
 		if (err)
-			goto error;
+			goto error_put_device;
 	}
 
 	if (phydev->attached_dev) {

@@ -981,6 +981,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error:
 	phy_detach(phydev);
+error_put_device:
 	put_device(d);
 	module_put(d->driver->owner);
 	if (ndev_owner != bus->owner)

@@ -1065,7 +1066,8 @@ void phy_detach(struct phy_device *phydev)
 	bus = phydev->mdio.bus;
 
 	put_device(&phydev->mdio.dev);
-	module_put(phydev->mdio.dev.driver->owner);
+	if (phydev->mdio.dev.driver)
+		module_put(phydev->mdio.dev.driver->owner);
 	if (ndev_owner != bus->owner)
 		module_put(bus->owner);
 }

-- 
2.9.3

In preparation for supporting driver bind/unbind properly, sprinkle checks on
phydev->drv where we may call into PHYLIB from user-space or other parts of the
kernel.

Suggested-by: Russell King <redacted>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c | 26 ++++++++++++++++++++++----
 include/linux/phy.h   |  3 +++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7dcfe05..d6f7838455dd 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -580,7 +580,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		return 0;
 
 	case SIOCSHWTSTAMP:
-		if (phydev->drv->hwtstamp)
+		if (phydev->drv && phydev->drv->hwtstamp)
 			return phydev->drv->hwtstamp(phydev, ifr);
 		/* fall through */
 

@@ -603,6 +603,9 @@ int phy_start_aneg(struct phy_device *phydev)
 {
 	int err;
 
+	if (!phydev->drv)
+		return -EIO;
+
 	mutex_lock(&phydev->lock);
 
 	if (AUTONEG_DISABLE == phydev->autoneg)

@@ -975,7 +978,7 @@ void phy_state_machine(struct work_struct *work)
 
 	old_state = phydev->state;
 
-	if (phydev->drv->link_change_notify)
+	if (phydev->drv && phydev->drv->link_change_notify)
 		phydev->drv->link_change_notify(phydev);
 
 	switch (phydev->state) {

@@ -1286,6 +1289,9 @@ EXPORT_SYMBOL(phy_write_mmd_indirect);
  */
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 {
+	if (!phydev->drv)
+		return -EIO;
+
 	/* According to 802.3az,the EEE is supported only in full duplex-mode.
 	 * Also EEE feature is active when core is operating with MII, GMII
 	 * or RGMII (all kinds). Internal PHYs are also allowed to proceed and

@@ -1363,6 +1369,9 @@ EXPORT_SYMBOL(phy_init_eee);
  */
 int phy_get_eee_err(struct phy_device *phydev)
 {
+	if (!phydev->drv)
+		return -EIO;
+
 	return phy_read_mmd_indirect(phydev, MDIO_PCS_EEE_WK_ERR, MDIO_MMD_PCS);
 }
 EXPORT_SYMBOL(phy_get_eee_err);

@@ -1379,6 +1388,9 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
 	int val;
 
+	if (!phydev->drv)
+		return -EIO;
+
 	/* Get Supported EEE */
 	val = phy_read_mmd_indirect(phydev, MDIO_PCS_EEE_ABLE, MDIO_MMD_PCS);
 	if (val < 0)

@@ -1412,6 +1424,9 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
 	int val = ethtool_adv_to_mmd_eee_adv_t(data->advertised);
 
+	if (!phydev->drv)
+		return -EIO;
+
 	/* Mask prohibited EEE modes */
 	val &= ~phydev->eee_broken_modes;
 

@@ -1423,7 +1438,7 @@ EXPORT_SYMBOL(phy_ethtool_set_eee);
 
 int phy_ethtool_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
-	if (phydev->drv->set_wol)
+	if (phydev->drv && phydev->drv->set_wol)
 		return phydev->drv->set_wol(phydev, wol);
 
 	return -EOPNOTSUPP;

@@ -1432,7 +1447,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
 
 void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
-	if (phydev->drv->get_wol)
+	if (phydev->drv && phydev->drv->get_wol)
 		phydev->drv->get_wol(phydev, wol);
 }
 EXPORT_SYMBOL(phy_ethtool_get_wol);

@@ -1468,6 +1483,9 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
 	if (!phydev)
 		return -ENODEV;
 
+	if (!phydev->drv)
+		return -EIO;
+
 	return genphy_restart_aneg(phydev);
 }
 EXPORT_SYMBOL(phy_ethtool_nway_reset);

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7fc1105605bf..231e07bb0d76 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -802,6 +802,9 @@ int phy_stop_interrupts(struct phy_device *phydev);
 
 static inline int phy_read_status(struct phy_device *phydev)
 {
+	if (!phydev->drv)
+		return -EIO;
+
 	return phydev->drv->read_status(phydev);
 }
 

-- 
2.9.3

The PHY library does not deal very well with bind and unbind events. The first
thing we would see is that we were not properly canceling the PHY state machine
workqueue, so we would be crashing while dereferencing phydev->drv since there
is no driver attached anymore.

Once we fix that, there are several things that did not quite work as expected:

- if the PHY state machine was running, we were not stopping it properly, and
  the state machine state would not be marked as such
- when we rebind the driver, nothing would happen, since we would not know which
  state we were before the unbind

This patch takes the following approach:

- if the PHY was attached, and the state machine was running we would stop it,
  remember where we left, and schedule the state machine for restart upong
  driver bind
- if the PHY was attached, but HALTED, we would let it in that state, and do not
  alter the state upon driver bind
- in all other cases (detached) we would keep the PHY in DOWN state waiting for
  a network driver to show up, and set PHY_READY on driver bind

Suggested-by: Russell King <redacted>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bde240bf8d7b..5314e764a387 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -1711,6 +1711,7 @@ static int phy_probe(struct device *dev)
 	struct phy_device *phydev = to_phy_device(dev);
 	struct device_driver *drv = phydev->mdio.dev.driver;
 	struct phy_driver *phydrv = to_phy_driver(drv);
+	bool should_start = false;
 	int err = 0;
 
 	phydev->drv = phydrv;

@@ -1760,24 +1761,46 @@ static int phy_probe(struct device *dev)
 	}
 
 	/* Set the state to READY by default */
-	phydev->state = PHY_READY;
+	if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
+		should_start = true;
+	else
+		phydev->state = PHY_READY;
 
 	if (phydev->drv->probe)
 		err = phydev->drv->probe(phydev);
 
 	mutex_unlock(&phydev->lock);
 
+	if (should_start)
+		phy_start(phydev);
+
 	return err;
 }
 
 static int phy_remove(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
+	bool should_stop = false;
+	enum phy_state state;
+
+	cancel_delayed_work_sync(&phydev->state_queue);
 
 	mutex_lock(&phydev->lock);
-	phydev->state = PHY_DOWN;
+	state = phydev->state;
+	if (state > PHY_UP && state != PHY_HALTED)
+		should_stop = true;
+	else
+		phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
 
+	/* phy_stop() sets the state to HALTED, undo that for the ->probe() function
+	 * to have a chance to resume where we left
+	 */
+	if (should_stop) {
+		phy_stop(phydev);
+		phydev->state = state;
+	}
+
 	if (phydev->drv->remove)
 		phydev->drv->remove(phydev);
 	phydev->drv = NULL;

-- 
2.9.3

Hi Florian,
  Here "return -EIO" will miss decreasing bus->owner reference count.

Mao Wenan

On Wed, Feb 8, 2017 at 5:37 AM, Florian Fainelli [off-list ref] wrote:

The Generic PHY drivers gets assigned after we checked that the current PHY
driver is NULL, so we need to check a few things before we can safely
derference d->driver. Update phy_attach_direct() and phy_detach() accordingly
to be resilient to these cases.

Even though the Generic PHY driver defaults to phy_probe() which can hardly
fail at the moment, let's fix the label so we don't call phy_detach() on a
network device we have not attached yet.

Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

This fixes the kernel crash:

Tested-by: Fabio Estevam <redacted>

Can we get into problems here? Maybe we did not do a get?

Humm. By this point, havn't we assigned d->driver to genphy_driver?
So this decrements something we never incremented?

To me, it seems better to move the try_module_get() after assigning
genphy_driver if needed. We then don't have any special conditions.

      Andrew

On 02/08/2017 08:57 AM, Andrew Lunn wrote:

quoted

--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -920,7 +920,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		return -EIO;
 	}
 
-	if (!try_module_get(d->driver->owner)) {
+	if (d->driver && !try_module_get(d->driver->owner)) {
 		dev_err(&dev->dev, "failed to get the device driver module\n");
 		return -EIO;
 	}

@@ -943,7 +943,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			err = device_bind_driver(d);
 
 		if (err)
-			goto error;
+			goto error_put_device;
 	}
 
 	if (phydev->attached_dev) {

@@ -981,6 +981,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error:
 	phy_detach(phydev);
+error_put_device:
 	put_device(d);
 	module_put(d->driver->owner);

Can we get into problems here? Maybe we did not do a get?

Humm yes, that seems possible too.

quoted

 	if (ndev_owner != bus->owner)

@@ -1065,7 +1066,8 @@ void phy_detach(struct phy_device *phydev)
 	bus = phydev->mdio.bus;
 
 	put_device(&phydev->mdio.dev);
-	module_put(phydev->mdio.dev.driver->owner);
+	if (phydev->mdio.dev.driver)
+		module_put(phydev->mdio.dev.driver->owner);

Humm. By this point, havn't we assigned d->driver to genphy_driver?
So this decrements something we never incremented?

Not really, the generic PHY drivers are built into libphy, so all PHY
drivers de facto going to depend on libphy.ko being loaded first, that's
why we can take shortcuts and not increment the refcount on the
genphy/libphy module here.

To me, it seems better to move the try_module_get() after assigning
genphy_driver if needed. We then don't have any special conditions.

Sounds fair, let me fix that (and the problem reported by Mao about
error handling).

Thanks!
-- 
Florian

On 02/07/2017 11:57 PM, maowenan wrote:

quoted

-----Original Message-----
From: Florian Fainelli [mailto:f.fainelli@gmail.com]
Sent: Wednesday, February 08, 2017 3:38 PM
To: netdev@vger.kernel.org
Cc: davem@davemloft.net; andrew@lunn.ch; rmk+kernel@armlinux.org.uk;
maowenan; Florian Fainelli
Subject: [PATCH net v2 1/3] net: phy: Fix PHY module checks

The Generic PHY drivers gets assigned after we checked that the current PHY
driver is NULL, so we need to check a few things before we can safely
derference d->driver. Update phy_attach_direct() and phy_detach() accordingly
to be resilient to these cases.

Even though the Generic PHY driver defaults to phy_probe() which can hardly
fail at the moment, let's fix the label so we don't call phy_detach() on a network
device we have not attached yet.

Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index

0d8f4d3847f6..bde240bf8d7b 100644

--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -920,7 +920,7 @@ int phy_attach_direct(struct net_device *dev, struct

phy_device *phydev,
 		return -EIO;
 	}

-	if (!try_module_get(d->driver->owner)) {
+	if (d->driver && !try_module_get(d->driver->owner)) {
 		dev_err(&dev->dev, "failed to get the device driver module\n");
 		return -EIO;

Hi Florian,
  Here "return -EIO" will miss decreasing bus->owner reference count.

Good catch thanks!

-- 
Florian