stmmac: call stmmac_init_phy from stmmac_dvr_probe

PATCH In review Niklas Cassel 3 participants 9 messages Mar 20, 2017 – Mar 21, 2017

Subsystems (4)

Networking Network drivers Arm STM32 stmmac Ethernet driver

lore

Share & tools

lore

Niklas Cassel

author

2017-03-20 17:29 UTC #0 lore raw

PATCH 1/1 stmmac: call stmmac_init_phy from stmmac_dvr_probe

From: Niklas Cassel <[email protected]>

It is usually possible to do
ethtool -s autoneg on
so that you trigger an autoneg before calling
ip link set dev eth0 up

However, stmmac returns -EBUSY if !netif_running. The only reason for this appears to be that stmmac_init_phy is called from stmmac_open instead of from stmmac_dvr_probe.

Move stmmac_init_phy to stmmac_dvr_probe so that ethool works as soon as register_netdev has been called. stmmac_check_ether_addr was also moved to probe, so that the ordering doesn't change.

Signed-off-by Niklas Cassel <[email protected]>

---
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 13 --------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 35 +++++++++-------------
 2 files changed, 14 insertions(+), 34 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +0 ~~-13~~

index 16808e48ca1c..027c280f33d0 100644
@@ -359,11 +359,6 @@ static int stmmac_ethtool_get_link_ksettings(struct net_device *dev,
 		       __func__, dev->name);
 		return -ENODEV;
 	}
-	if (!netif_running(dev)) {
-		pr_err("%s: interface is disabled: we cannot track "
-		"link speed / duplex setting\n", dev->name);
-		return -EBUSY;
-	}
 	rc = phy_ethtool_ksettings_get(phy, cmd);
 	return rc;
 }

@@ -420,13 +415,6 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level)
 
 }
 
-static int stmmac_check_if_running(struct net_device *dev)
-{
-	if (!netif_running(dev))
-		return -EBUSY;
-	return 0;
-}
-
 static int stmmac_ethtool_get_regs_len(struct net_device *dev)
 {
 	return REG_SPACE_SIZE;

@@ -847,7 +835,6 @@ static int stmmac_set_tunable(struct net_device *dev,
 }
 
 static const struct ethtool_ops stmmac_ethtool_ops = {
-	.begin = stmmac_check_if_running,
 	.get_drvinfo = stmmac_ethtool_getdrvinfo,
 	.get_msglevel = stmmac_ethtool_getmsglevel,
 	.set_msglevel = stmmac_ethtool_setmsglevel,

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +14 ~~-21~~

index d3a21519e4c0..7335ae60dc70 100644
@@ -2080,20 +2080,6 @@ static int stmmac_open(struct net_device *dev)
 	struct stmmac_priv *priv = netdev_priv(dev);
 	int ret;
 
-	stmmac_check_ether_addr(priv);
-
-	if (priv->hw->pcs != STMMAC_PCS_RGMII &&
-	    priv->hw->pcs != STMMAC_PCS_TBI &&
-	    priv->hw->pcs != STMMAC_PCS_RTBI) {
-		ret = stmmac_init_phy(dev);
-		if (ret) {
-			netdev_err(priv->dev,
-				   "%s: Cannot attach to PHY (error: %d)\n",
-				   __func__, ret);
-			return ret;
-		}
-	}
-
 	/* Extra statistics */
 	memset(&priv->xstats, 0, sizeof(struct stmmac_extra_stats));
 	priv->xstats.threshold = tc;

@@ -2179,9 +2165,6 @@ static int stmmac_open(struct net_device *dev)
 init_error:
 	free_dma_desc_resources(priv);
 dma_desc_error:
-	if (dev->phydev)
-		phy_disconnect(dev->phydev);
-
 	return ret;
 }

@@ -2198,11 +2181,8 @@ static int stmmac_release(struct net_device *dev)
 	if (priv->eee_enabled)
 		del_timer_sync(&priv->eee_ctrl_timer);
 
-	/* Stop and disconnect the PHY */
-	if (dev->phydev) {
+	if (dev->phydev)
 		phy_stop(dev->phydev);
-		phy_disconnect(dev->phydev);
-	}
 
 	netif_stop_queue(dev);

@@ -3656,6 +3636,7 @@ int stmmac_dvr_probe(struct device *device,
 		priv->clk_csr = priv->plat->clk_csr;
 
 	stmmac_check_pcs_mode(priv);
+	stmmac_check_ether_addr(priv);
 
 	if (priv->hw->pcs != STMMAC_PCS_RGMII  &&
 	    priv->hw->pcs != STMMAC_PCS_TBI &&

@@ -3668,6 +3649,13 @@ int stmmac_dvr_probe(struct device *device,
 				__func__, priv->plat->bus_id);
 			goto error_mdio_register;
 		}
+		ret = stmmac_init_phy(ndev);
+		if (ret) {
+			dev_err(priv->device,
+				"%s: Cannot attach to PHY (error: %d)\n",
+				__func__, ret);
+			goto error_init_phy;
+		}
 	}
 
 	ret = register_netdev(ndev);

@@ -3683,6 +3671,11 @@ int stmmac_dvr_probe(struct device *device,
 	if (priv->hw->pcs != STMMAC_PCS_RGMII &&
 	    priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI)
+		phy_disconnect(ndev->phydev);
+error_init_phy:
+	if (priv->hw->pcs != STMMAC_PCS_RGMII &&
+	    priv->hw->pcs != STMMAC_PCS_TBI &&
+	    priv->hw->pcs != STMMAC_PCS_RTBI)
 		stmmac_mdio_unregister(ndev);
 error_mdio_register:
 	netif_napi_del(&priv->napi);

Signature · 1 line

2.11.0

Joao Pinto

2017-03-20 17:42 UTC #1 lore raw

Às 5:29 PM de 3/20/2017, Niklas Cassel escreveu:

Quoted 11 lines

From: Niklas Cassel <[email protected]>
It is usually possible to do
ethtool -s autoneg on
so that you trigger an autoneg before calling
ip link set dev eth0 up
However, stmmac returns -EBUSY if !netif_running. The only reason for this appears to be that stmmac_init_phy is called from stmmac_open instead of from stmmac_dvr_probe.

Quoted 7 lines

Move stmmac_init_phy to stmmac_dvr_probe so that ethool works as soon as register_netdev has been called. stmmac_check_ether_addr was also moved to probe, so that the ordering doesn't change.
Signed-off-by Niklas Cassel <[email protected]>
---

Hi Niklas, did you test this patch?

Thanks.
Joao

Niklas Cassel

author

2017-03-20 17:46 UTC #2 lore raw

Quoted 12 lines · Joao Pinto

Às 5:29 PM de 3/20/2017, Niklas Cassel escreveu:
Quoted 11 lines
From: Niklas Cassel <[email protected]>
It is usually possible to do
ethtool -s autoneg on
so that you trigger an autoneg before calling
ip link set dev eth0 up
However, stmmac returns -EBUSY if !netif_running. The only reason for this appears to be that stmmac_init_phy is called from stmmac_open instead of from stmmac_dvr_probe.

Quoted 8 lines

Quoted 7 lines
Move stmmac_init_phy to stmmac_dvr_probe so that ethool works as soon as register_netdev has been called. stmmac_check_ether_addr was also moved to probe, so that the ordering doesn't change.
Signed-off-by Niklas Cassel <[email protected]>
---
Hi Niklas, did you test this patch?

I tested it on our hardware, but it would be nice
if you could test it on your side, since you are
using the PCI glue layer.

Joao Pinto

2017-03-20 17:46 UTC #3 lore raw

Às 5:44 PM de 3/20/2017, Niklas Cassel escreveu:

Quoted 18 lines · Joao Pinto

Quoted 17 lines
Às 5:29 PM de 3/20/2017, Niklas Cassel escreveu:
Quoted 16 lines
From: Niklas Cassel <[email protected]>
It is usually possible to do
ethtool -s autoneg on
so that you trigger an autoneg before calling
ip link set dev eth0 up
However, stmmac returns -EBUSY if !netif_running. The only reason for this appears to be that stmmac_init_phy is called from stmmac_open instead of from stmmac_dvr_probe.
Move stmmac_init_phy to stmmac_dvr_probe so that ethool works as soon as register_netdev has been called. stmmac_check_ether_addr was also moved to probe, so that the ordering doesn't change.

Quoted 8 lines

Quoted 3 lines
Quoted 2 lines
Signed-off-by Niklas Cassel <[email protected]>
---
Hi Niklas, did you test this patch?
I tested it on our hardware, but it would be nice
if you could test it on your side, since you are
using the PCI glue layer.

Ok, I can do that. As Florian stated in a past e-mail, this change might not be compatible with all setups.

Joao

Florian Fainelli

2017-03-20 17:43 UTC #4 lore raw

In reply to Niklas Cassel

Quoted 6 lines · Niklas Cassel

From: Niklas Cassel <[email protected]>
It is usually possible to do
ethtool -s autoneg on
so that you trigger an autoneg before calling
ip link set dev eth0 up

This is completely driver specific and there is no guarantee for this to work universally across all device drivers because when your interface is brought down, the most sensible thing to expect in return is that your PHY is powered down (unless your interface participates in Wake-on-LAN).

Quoted 9 lines

However, stmmac returns -EBUSY if !netif_running. The only reason for this appears to be that stmmac_init_phy is called from stmmac_open instead of from stmmac_dvr_probe.
Move stmmac_init_phy to stmmac_dvr_probe so that ethool works as soon as register_netdev has been called. stmmac_check_ether_addr was also moved to probe, so that the ordering doesn't change.

Are you sure this is a good idea? There are many drivers that moved the PHY probe into ndo_open() for mainly two things:

- phy_connect() starts the PHY state machine and starting the state machine without a network device running is kind of wasting cycles

- if the interface is probed, but not used, you are keeping the Ethernet link running without being able to service packets, which is at best a waste of power
--
Florian

Niklas Cassel

author

2017-03-20 18:34 UTC #5 lore raw

Quoted 24 lines · Florian Fainelli

Quoted 6 lines · Niklas Cassel
From: Niklas Cassel <[email protected]>
It is usually possible to do
ethtool -s autoneg on
so that you trigger an autoneg before calling
ip link set dev eth0 up
This is completely driver specific and there is no guarantee for this to work universally across all device drivers because when your interface is brought down, the most sensible thing to expect in return is that your PHY is powered down (unless your interface participates in Wake-on-LAN).
Quoted 8 lines
However, stmmac returns -EBUSY if !netif_running. The only reason for this appears to be that stmmac_init_phy is called from stmmac_open instead of from stmmac_dvr_probe.
Move stmmac_init_phy to stmmac_dvr_probe so that ethool works as soon as register_netdev has been called. stmmac_check_ether_addr was also moved to probe, so that the ordering doesn't change.
Are you sure this is a good idea? There are many drivers that moved the PHY probe into ndo_open() for mainly two things:

Quoted 6 lines

- phy_connect() starts the PHY state machine and starting the state machine without a network device running is kind of wasting cycles
- if the interface is probed, but not used, you are keeping the Ethernet link running without being able to service packets, which is at best a waste of power

Hello Florian

Thank you for your input.
I can see the point in keeping phy_connect in ndo_open.

What I dislike is the -EBUSY from stmmac_ethtool_get_link_ksettings, since this will create warnings in user space by our favorite monolith. (Please don't flame me, I dislike it as much as you guys.)

[ WARNING ] systemd-udevd[236]: link_config: could not get ethtool features for eth0 [ WARNING ] systemd-udevd[236]: Could not set offload features of eth0: Device or resource busy

However, it is kind of sad that drivers are so inconsistent of what goes in probe and what goes in ndo_open...which is tied together with the whole mess of when certain ethtool commands work or do not work.

Do you know of a good way to avoid the -EBUSY in stmmac_ethtool_get_link_ksettings, but still keep phy_connect in ndo_open?
The current code checks netif_running(), which checks __LINK_STATE_START, which gets set by __dev_open().
stmmac_ethtool_get_link_ksettings also returns -ENODEV if ndev->phydev == NULL.

Florian Fainelli

2017-03-20 22:07 UTC #6 lore raw

Quoted 37 lines · Niklas Cassel

Quoted 24 lines · Florian Fainelli
Quoted 6 lines · Niklas Cassel
From: Niklas Cassel <[email protected]>
It is usually possible to do
ethtool -s autoneg on
so that you trigger an autoneg before calling
ip link set dev eth0 up
This is completely driver specific and there is no guarantee for this to work universally across all device drivers because when your interface is brought down, the most sensible thing to expect in return is that your PHY is powered down (unless your interface participates in Wake-on-LAN).
Quoted 8 lines
However, stmmac returns -EBUSY if !netif_running. The only reason for this appears to be that stmmac_init_phy is called from stmmac_open instead of from stmmac_dvr_probe.
Move stmmac_init_phy to stmmac_dvr_probe so that ethool works as soon as register_netdev has been called. stmmac_check_ether_addr was also moved to probe, so that the ordering doesn't change.
Are you sure this is a good idea? There are many drivers that moved the PHY probe into ndo_open() for mainly two things:
Quoted 6 lines
- phy_connect() starts the PHY state machine and starting the state machine without a network device running is kind of wasting cycles
- if the interface is probed, but not used, you are keeping the Ethernet link running without being able to service packets, which is at best a waste of power
Hello Florian
Thank you for your input.
I can see the point in keeping phy_connect in ndo_open.

Quoted 6 lines

What I dislike is the -EBUSY from stmmac_ethtool_get_link_ksettings, since this will create warnings in user space by our favorite monolith. (Please don't flame me, I dislike it as much as you guys.)
[ WARNING ] systemd-udevd[236]: link_config: could not get ethtool features for eth0 [ WARNING ] systemd-udevd[236]: Could not set offload features of eth0: Device or resource busy

Then let's silence the warning, because it's not really helpful in the first place.

Quoted 5 lines

However, it is kind of sad that drivers are so inconsistent of what goes in probe and what goes in ndo_open...which is tied together with the whole mess of when certain ethtool commands work or do not work.

Well, inconsistent here is kind of big statement, what I meant to say is that your proposed change actually makes thing inconsistent.

Quoted 3 lines

Do you know of a good way to avoid the -EBUSY in stmmac_ethtool_get_link_ksettings, but still keep phy_connect in ndo_open?

Let me rephrase this: doing an ethtool operation on an interface that is not open is not a defined behavior which adheres to a contract with the kernel. ethtool operations on interfaces that are DOWN, may succeed if they do not involve a piece of hardware that needs to be active (e.g:
the PHY) but there is no guarantee that a) the change is immediately applied, or b) that the results are consistent.

The best thing to do IMHO is just silence the warning, return an error coed, and come back configuring the interface at a later time when it is guaranteed to be operational.

Quoted 4 lines

The current code checks netif_running(), which checks __LINK_STATE_START, which gets set by __dev_open().
stmmac_ethtool_get_link_ksettings also returns -ENODEV if ndev->phydev == NULL.

Signature · 1 line

Florian

Niklas Cassel

author

2017-03-21 09:04 UTC #7 lore raw

Quoted 8 lines · Florian Fainelli

(snip)
Quoted 4 lines
However, it is kind of sad that drivers are so inconsistent of what goes in probe and what goes in ndo_open...which is tied together with the whole mess of when certain ethtool commands work or do not work.
Well, inconsistent here is kind of big statement, what I meant to say is that your proposed change actually makes thing inconsistent.

What I mean is that it is about 50/50 if something e.g. phy_connect is called from probe or from ndo_open.
This is what I think is inconsistent.

git grep -p -E "phy_connect\(|phy_connect_direct\(" drivers/net/ethernet/ | grep open | wc -l 12

git grep -p -E "phy_connect\(|phy_connect_direct\(" drivers/net/ethernet/ | grep probe | wc -l 27
even if we exclude *_mii_probe/*_mdio_probe which are done
git grep -p mii_probe drivers/net/ethernet/ | grep open | wc -l
6
git grep -p mdio_probe drivers/net/ethernet/ | grep open | wc -l
2

phy_connect done from probe:
27-6-2=19

phy_connect done from ndo_open:
12+6+2=20

Quoted 10 lines

Quoted 3 lines
Do you know of a good way to avoid the -EBUSY in stmmac_ethtool_get_link_ksettings, but still keep phy_connect in ndo_open?
Let me rephrase this: doing an ethtool operation on an interface that is not open is not a defined behavior which adheres to a contract with the kernel. ethtool operations on interfaces that are DOWN, may succeed if they do not involve a piece of hardware that needs to be active (e.g:
the PHY) but there is no guarantee that a) the change is immediately applied, or b) that the results are consistent.

Quoted 3 lines

The best thing to do IMHO is just silence the warning, return an error coed, and come back configuring the interface at a later time when it is guaranteed to be operational.

Thank you for your feedback. I agree with you, silencing the warning would be the best way forward. David, please ignore this patch.

However, since ethtool is a user interface, I think that it would have been nice if it wasn't so driver specific regarding to if a command works before the interface is open or not.

If the user does some ethtool operation that affects e.g. the PHY before the interface is open, perhaps there is a way to save this setting so it could be applied when the resource is available.

For the PHY case, net_device has a pointer to a phy_device, register_netdev could create a dummy phy_device if ndev->phy_device is NULL, that way changes could be saved and applied when phy_connect replaces the dummy device
(or perhaps when phy_start() is called).
(If it isn't a dummy phy_device, changes could be applied immediately.) This way it would not matter if phy_connect is done from probe or from ndo_open.

Florian Fainelli

2017-03-21 16:32 UTC #8 lore raw

Quoted 52 lines · Niklas Cassel

Quoted 8 lines · Florian Fainelli
(snip)
Quoted 4 lines
However, it is kind of sad that drivers are so inconsistent of what goes in probe and what goes in ndo_open...which is tied together with the whole mess of when certain ethtool commands work or do not work.
Well, inconsistent here is kind of big statement, what I meant to say is that your proposed change actually makes thing inconsistent.
What I mean is that it is about 50/50 if something e.g. phy_connect is called from probe or from ndo_open.
This is what I think is inconsistent.
git grep -p -E "phy_connect\(|phy_connect_direct\(" drivers/net/ethernet/ | grep open | wc -l 12
git grep -p -E "phy_connect\(|phy_connect_direct\(" drivers/net/ethernet/ | grep probe | wc -l 27
even if we exclude *_mii_probe/*_mdio_probe which are done
git grep -p mii_probe drivers/net/ethernet/ | grep open | wc -l
6
git grep -p mdio_probe drivers/net/ethernet/ | grep open | wc -l
2
phy_connect done from probe:
27-6-2=19
phy_connect done from ndo_open:
12+6+2=20
Quoted 10 lines
Quoted 3 lines
Do you know of a good way to avoid the -EBUSY in stmmac_ethtool_get_link_ksettings, but still keep phy_connect in ndo_open?
Let me rephrase this: doing an ethtool operation on an interface that is not open is not a defined behavior which adheres to a contract with the kernel. ethtool operations on interfaces that are DOWN, may succeed if they do not involve a piece of hardware that needs to be active (e.g:
the PHY) but there is no guarantee that a) the change is immediately applied, or b) that the results are consistent.
Quoted 3 lines
The best thing to do IMHO is just silence the warning, return an error coed, and come back configuring the interface at a later time when it is guaranteed to be operational.
Thank you for your feedback. I agree with you, silencing the warning would be the best way forward. David, please ignore this patch.

Quoted 3 lines

However, since ethtool is a user interface, I think that it would have been nice if it wasn't so driver specific regarding to if a command works before the interface is open or not.

On premise I agree, in practice, every HW and driver is different, and enforcing standard behaviors is even more difficult as there are multiple pieces of HW interconnected to each other: MAC, MDIO bus, PHY etc. all with different power management capabilities and properties, and different requirements, and of course, varying degrees of brokenness.

Quoted 4 lines

If the user does some ethtool operation that affects e.g. the PHY before the interface is open, perhaps there is a way to save this setting so it could be applied when the resource is available.

Well behaving driver will connect to the PHY as one of their final driver's ndo_open() step, any call in between will result in phydev being NULL which will return a proper error code and should be acted upon by the application doing the ethtool calls (or the one doing ioctl() calls similar to ethtool).

Quoted 10 lines

For the PHY case, net_device has a pointer to a phy_device, register_netdev could create a dummy phy_device if ndev->phy_device is NULL, that way changes could be saved and applied when phy_connect replaces the dummy device
(or perhaps when phy_start() is called).
(If it isn't a dummy phy_device, changes could be applied immediately.) This way it would not matter if phy_connect is done from probe or from ndo_open.

I don't think there is a need for a dummy PHY device, you can do this in your driver if you want to, but this adds a lot of complexity for little value, and, keep in mind that before ndo_open() is complete, most operations against your network driver can be undefined.

Signature · 1 line

Florian