| Message ID | 20170320172915.8313-1-niklass@axis.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Às 5:29 PM de 3/20/2017, Niklas Cassel escreveu: > From: Niklas Cassel <niklas.cassel@axis.com> > > 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 <niklas.cassel@axis.com> > --- Hi Niklas, did you test this patch? Thanks. Joao
On 03/20/2017 10:29 AM, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@axis.com> > > 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). > > 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
On 03/20/2017 06:42 PM, Joao Pinto wrote: > Às 5:29 PM de 3/20/2017, Niklas Cassel escreveu: >> From: Niklas Cassel <niklas.cassel@axis.com> >> >> 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 <niklas.cassel@axis.com> >> --- > 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.
Às 5:44 PM de 3/20/2017, Niklas Cassel escreveu: > On 03/20/2017 06:42 PM, Joao Pinto wrote: >> Às 5:29 PM de 3/20/2017, Niklas Cassel escreveu: >>> From: Niklas Cassel <niklas.cassel@axis.com> >>> >>> 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 <niklas.cassel@axis.com> >>> --- >> 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
On 03/20/2017 06:43 PM, Florian Fainelli wrote: > On 03/20/2017 10:29 AM, Niklas Cassel wrote: >> From: Niklas Cassel <niklas.cassel@axis.com> >> >> 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). > >> 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 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.
On 03/20/2017 11:34 AM, Niklas Cassel wrote: > On 03/20/2017 06:43 PM, Florian Fainelli wrote: >> On 03/20/2017 10:29 AM, Niklas Cassel wrote: >>> From: Niklas Cassel <niklas.cassel@axis.com> >>> >>> 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). >> >>> 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 > > 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 Then let's silence the warning, because it's not really helpful in the first place. > > > 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. > > 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. > 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. >
On 03/20/2017 11:07 PM, Florian Fainelli wrote: > > (snip) >> >> 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 > >> 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. 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.
On 03/21/2017 02:04 AM, Niklas Cassel wrote: > On 03/20/2017 11:07 PM, Florian Fainelli wrote: >> >> (snip) >>> >>> 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 > > >> >>> 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. > > 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. 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. > > 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). > > 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.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 16808e48ca1c..027c280f33d0 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -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, diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index d3a21519e4c0..7335ae60dc70 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -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);