| Message ID | 20240223094425.691209-6-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Rework EEE | expand |
On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > + struct ethtool_keee *data) > +{ > + if (phydev->eee_cfg.tx_lpi_enabled != > + data->tx_lpi_enabled) { > + eee_to_eeecfg(data, &phydev->eee_cfg); > + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); > + if (phydev->link) > + phy_link_up(phydev); I'm not convinced this is a good idea. Hasn't phylib previously had the guarantee that the link will go down between two link-up events? So calling phy_link_up() may result in either the MAC driver ignoring it, or modifying registers that are only supposed to be modified while the MAC side is down.
On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote: > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > > + struct ethtool_keee *data) > > +{ > > + if (phydev->eee_cfg.tx_lpi_enabled != > > + data->tx_lpi_enabled) { > > + eee_to_eeecfg(data, &phydev->eee_cfg); > > + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); > > + if (phydev->link) > > + phy_link_up(phydev); > > I'm not convinced this is a good idea. Hasn't phylib previously had > the guarantee that the link will go down between two link-up events? > So calling phy_link_up() may result in either the MAC driver ignoring > it, or modifying registers that are only supposed to be modified while > the MAC side is down. Ok, I'll drop this patch. Regards, Oleksij
On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote: > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > > + struct ethtool_keee *data) > > +{ > > + if (phydev->eee_cfg.tx_lpi_enabled != > > + data->tx_lpi_enabled) { > > + eee_to_eeecfg(data, &phydev->eee_cfg); > > + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); > > + if (phydev->link) > > + phy_link_up(phydev); > > I'm not convinced this is a good idea. Hasn't phylib previously had > the guarantee that the link will go down between two link-up events? > So calling phy_link_up() may result in either the MAC driver ignoring > it, or modifying registers that are only supposed to be modified while > the MAC side is down. When auto-neg is used, we expect the link to go down and come back up again. Here we are dealing with the case that autoneg is not used. The MAC needs informing somehow. If we want to preserve the down/up, we could call phy_link_down() and then phy_link_up() back to back. Andrew
On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote: > On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote: > > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: > > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > > > + struct ethtool_keee *data) > > > +{ > > > + if (phydev->eee_cfg.tx_lpi_enabled != > > > + data->tx_lpi_enabled) { > > > + eee_to_eeecfg(data, &phydev->eee_cfg); > > > + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); > > > + if (phydev->link) > > > + phy_link_up(phydev); > > > > I'm not convinced this is a good idea. Hasn't phylib previously had > > the guarantee that the link will go down between two link-up events? > > So calling phy_link_up() may result in either the MAC driver ignoring > > it, or modifying registers that are only supposed to be modified while > > the MAC side is down. > > When auto-neg is used, we expect the link to go down and come back up > again. > > Here we are dealing with the case that autoneg is not used. The MAC > needs informing somehow. If we want to preserve the down/up, we could > call phy_link_down() and then phy_link_up() back to back. Would it be better to have a separate callback for EEE state (as I mentioned in another comment on this series?) That would be better for future SmartEEE support.
On 2/23/24 05:26, Russell King (Oracle) wrote: > On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote: >> On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote: >>> On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: >>>> +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, >>>> + struct ethtool_keee *data) >>>> +{ >>>> + if (phydev->eee_cfg.tx_lpi_enabled != >>>> + data->tx_lpi_enabled) { >>>> + eee_to_eeecfg(data, &phydev->eee_cfg); >>>> + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); >>>> + if (phydev->link) >>>> + phy_link_up(phydev); >>> >>> I'm not convinced this is a good idea. Hasn't phylib previously had >>> the guarantee that the link will go down between two link-up events? >>> So calling phy_link_up() may result in either the MAC driver ignoring >>> it, or modifying registers that are only supposed to be modified while >>> the MAC side is down. >> >> When auto-neg is used, we expect the link to go down and come back up >> again. >> >> Here we are dealing with the case that autoneg is not used. The MAC >> needs informing somehow. If we want to preserve the down/up, we could >> call phy_link_down() and then phy_link_up() back to back. > > Would it be better to have a separate callback for EEE state (as I > mentioned in another comment on this series?) That would be better > for future SmartEEE support. That sounds like a good approach to me. The additional callback also helps figure out which drivers use the API and it should be simpler to audit for changes in the future, too.
On Fri, Feb 23, 2024 at 09:53:06AM -0800, Florian Fainelli wrote: > On 2/23/24 05:26, Russell King (Oracle) wrote: > > On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote: > > > On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote: > > > > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: > > > > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > > > > > + struct ethtool_keee *data) > > > > > +{ > > > > > + if (phydev->eee_cfg.tx_lpi_enabled != > > > > > + data->tx_lpi_enabled) { > > > > > + eee_to_eeecfg(data, &phydev->eee_cfg); > > > > > + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); > > > > > + if (phydev->link) > > > > > + phy_link_up(phydev); > > > > > > > > I'm not convinced this is a good idea. Hasn't phylib previously had > > > > the guarantee that the link will go down between two link-up events? > > > > So calling phy_link_up() may result in either the MAC driver ignoring > > > > it, or modifying registers that are only supposed to be modified while > > > > the MAC side is down. > > > > > > When auto-neg is used, we expect the link to go down and come back up > > > again. > > > > > > Here we are dealing with the case that autoneg is not used. The MAC > > > needs informing somehow. If we want to preserve the down/up, we could > > > call phy_link_down() and then phy_link_up() back to back. > > > > Would it be better to have a separate callback for EEE state (as I > > mentioned in another comment on this series?) That would be better > > for future SmartEEE support. > > That sounds like a good approach to me. The additional callback also helps > figure out which drivers use the API and it should be simpler to audit for > changes in the future, too. At this point I need help to understand how to proceed. What exactly do you have in mind? Some description like following would be helpful: Add callback with name phy_link_set_eee(), which should be executed in function bla/blup.. Regards, Oleksij
On Sat, Feb 24, 2024 at 06:57:12PM +0100, Oleksij Rempel wrote: > On Fri, Feb 23, 2024 at 09:53:06AM -0800, Florian Fainelli wrote: > > On 2/23/24 05:26, Russell King (Oracle) wrote: > > > On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote: > > > > On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote: > > > > > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: > > > > > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > > > > > > + struct ethtool_keee *data) > > > > > > +{ > > > > > > + if (phydev->eee_cfg.tx_lpi_enabled != > > > > > > + data->tx_lpi_enabled) { > > > > > > + eee_to_eeecfg(data, &phydev->eee_cfg); > > > > > > + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); > > > > > > + if (phydev->link) > > > > > > + phy_link_up(phydev); > > > > > > > > > > I'm not convinced this is a good idea. Hasn't phylib previously had > > > > > the guarantee that the link will go down between two link-up events? > > > > > So calling phy_link_up() may result in either the MAC driver ignoring > > > > > it, or modifying registers that are only supposed to be modified while > > > > > the MAC side is down. > > > > > > > > When auto-neg is used, we expect the link to go down and come back up > > > > again. > > > > > > > > Here we are dealing with the case that autoneg is not used. The MAC > > > > needs informing somehow. If we want to preserve the down/up, we could > > > > call phy_link_down() and then phy_link_up() back to back. > > > > > > Would it be better to have a separate callback for EEE state (as I > > > mentioned in another comment on this series?) That would be better > > > for future SmartEEE support. > > > > That sounds like a good approach to me. The additional callback also helps > > figure out which drivers use the API and it should be simpler to audit for > > changes in the future, too. > > At this point I need help to understand how to proceed. > What exactly do you have in mind? Some description like following would > be helpful: > Add callback with name phy_link_set_eee(), which should be executed in > function bla/blup.. When i first did this patchset, SmartEEE was out of scope. One question we should decide is, is it still out of scope, and we should first get 'dumb' EEE fixed everywhere, and than come back and look at SmartEEE? Or do we want to consider SmartEEE now? The idea of this patchset was to push as much as possible down into phylib. The MAC needs to say it supports EEE. I left handling of the tx_lpi_timer to the MAC driver, because the PHY has nothing it can do with that value. phylib then just needs to tell the MAC to enable or disable EEE when autoneg has completed. That i made part of the adjust link callback because that is the only callback we have, and most developers seem to understand it, and the locking around it. However, it does get messy when EEE can change without an auto-neg, as pointed out here. If we are leaving SmartEEE out of scope for the moment, i would say just doing a down/up is sufficient, lets get this merged and all 'dumb' EEE fixed. If we want feature creep and to think about SmartEEE then we need a few changes in the overall design. We need to make eee_get and eee_set transparent to the MAC driver, since the PHY could be doing it all. So phylib needs to track tx_lpi_timer. If the MAC driver indicates it can do 'dumb' EEE we probably want to use that in preference to SmartEEE, since i guess the MAC can also save a little power in LPI mode. So the adjust link callback needs to say: Enable MAC EEE with this value of tx_lpi_timer, or turn off MAC EEE. When using SmartEEE it will never actually do either. The current phylib model is that adjust_link is the only callback, and the MAC driver peeks into phydev to find what it needs. I would probably stick to that model, and not add MAC callbacks. phylink is slightly different, mac_link_up() passes everything the MAC needs to know as parameters, so one of my patches adds an extra parameter to indicate if EEE should be enabled or disabled. That would need extending with the tx_lpi_timer value. Andrew
On 2/26/24 08:44, Andrew Lunn wrote: > On Sat, Feb 24, 2024 at 06:57:12PM +0100, Oleksij Rempel wrote: >> On Fri, Feb 23, 2024 at 09:53:06AM -0800, Florian Fainelli wrote: >>> On 2/23/24 05:26, Russell King (Oracle) wrote: >>>> On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote: >>>>> On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote: >>>>>> On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote: >>>>>>> +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, >>>>>>> + struct ethtool_keee *data) >>>>>>> +{ >>>>>>> + if (phydev->eee_cfg.tx_lpi_enabled != >>>>>>> + data->tx_lpi_enabled) { >>>>>>> + eee_to_eeecfg(data, &phydev->eee_cfg); >>>>>>> + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); >>>>>>> + if (phydev->link) >>>>>>> + phy_link_up(phydev); >>>>>> >>>>>> I'm not convinced this is a good idea. Hasn't phylib previously had >>>>>> the guarantee that the link will go down between two link-up events? >>>>>> So calling phy_link_up() may result in either the MAC driver ignoring >>>>>> it, or modifying registers that are only supposed to be modified while >>>>>> the MAC side is down. >>>>> >>>>> When auto-neg is used, we expect the link to go down and come back up >>>>> again. >>>>> >>>>> Here we are dealing with the case that autoneg is not used. The MAC >>>>> needs informing somehow. If we want to preserve the down/up, we could >>>>> call phy_link_down() and then phy_link_up() back to back. >>>> >>>> Would it be better to have a separate callback for EEE state (as I >>>> mentioned in another comment on this series?) That would be better >>>> for future SmartEEE support. >>> >>> That sounds like a good approach to me. The additional callback also helps >>> figure out which drivers use the API and it should be simpler to audit for >>> changes in the future, too. >> >> At this point I need help to understand how to proceed. >> What exactly do you have in mind? Some description like following would >> be helpful: >> Add callback with name phy_link_set_eee(), which should be executed in >> function bla/blup.. > > When i first did this patchset, SmartEEE was out of scope. One > question we should decide is, is it still out of scope, and we should > first get 'dumb' EEE fixed everywhere, and than come back and look at > SmartEEE? Or do we want to consider SmartEEE now? I believe the considerations about SmartEEE (which BTW requires a less QCA-centric name to be found) are somewhat orthogonal to the concerns from Russell here. The concern is that MAC drivers are not expecting to see the following sequence of calls: ->adjust_link(link == 0) ->adjust_link(link == 1) ->adjust_link(link == 1) The latter is somewhat unusual and could lead to some spectacular and unknown bugs to be discovered. This is even more surprising for drivers in that the TX LPI timer is typically programmed as part of the .set_eee() callback. > > The idea of this patchset was to push as much as possible down into > phylib. The MAC needs to say it supports EEE. I left handling of the > tx_lpi_timer to the MAC driver, because the PHY has nothing it can do > with that value. phylib then just needs to tell the MAC to enable or > disable EEE when autoneg has completed. That i made part of the adjust > link callback because that is the only callback we have, and most > developers seem to understand it, and the locking around it. However, > it does get messy when EEE can change without an auto-neg, as pointed > out here. > > If we are leaving SmartEEE out of scope for the moment, i would say > just doing a down/up is sufficient, lets get this merged and all > 'dumb' EEE fixed. > > If we want feature creep and to think about SmartEEE then we need a > few changes in the overall design. I am fine with tackling Smart EEE later on, as a matter of fact, I don't believe many, if any changes at all should be required to your patch series here. We would need to find a less QCA-centric name since SmartEEE and AutoGrEEEn are both trade marks, though QCA definitively did a better job here at claiming a name people could understand. > > We need to make eee_get and eee_set transparent to the MAC driver, > since the PHY could be doing it all. So phylib needs to track > tx_lpi_timer. If the MAC driver indicates it can do 'dumb' EEE we > probably want to use that in preference to SmartEEE, since i guess the > MAC can also save a little power in LPI mode. So the adjust link > callback needs to say: Enable MAC EEE with this value of tx_lpi_timer, > or turn off MAC EEE. When using SmartEEE it will never actually do > either. Agreed. > > The current phylib model is that adjust_link is the only callback, and > the MAC driver peeks into phydev to find what it needs. I would > probably stick to that model, and not add MAC callbacks. phylink is > slightly different, mac_link_up() passes everything the MAC needs to > know as parameters, so one of my patches adds an extra parameter to > indicate if EEE should be enabled or disabled. That would need > extending with the tx_lpi_timer value. This is the source of the concern, we don't know which MAC drivers we might end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully none, because they should be well written to only update the parameters that need updating, but who knows?
On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote: > This is the source of the concern, we don't know which MAC drivers we might > end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully > none, because they should be well written to only update the parameters that > need updating, but who knows? Just quickly... There are some (I went through a bunch.) They don't support EEE. I haven't been through all though, so there could be some which support EEE and where adjust_link() with phydev->link=true twice in a row could result in badness.
On Mon, Feb 26, 2024 at 05:53:31PM +0000, Russell King (Oracle) wrote: > On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote: > > This is the source of the concern, we don't know which MAC drivers we might > > end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully > > none, because they should be well written to only update the parameters that > > need updating, but who knows? > > Just quickly... There are some (I went through a bunch.) They don't > support EEE. I haven't been through all though, so there could be > some which support EEE and where adjust_link() with phydev->link=true > twice in a row could result in badness. So i think we all agree the MAC needs to see a down/up, even if the link itself never went down. Anything else is too risky and will probably break something somewhere. Andrew
On Mon, Feb 26, 2024 at 07:59:28PM +0100, Andrew Lunn wrote: > On Mon, Feb 26, 2024 at 05:53:31PM +0000, Russell King (Oracle) wrote: > > On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote: > > > This is the source of the concern, we don't know which MAC drivers we might > > > end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully > > > none, because they should be well written to only update the parameters that > > > need updating, but who knows? > > > > Just quickly... There are some (I went through a bunch.) They don't > > support EEE. I haven't been through all though, so there could be > > some which support EEE and where adjust_link() with phydev->link=true > > twice in a row could result in badness. > > So i think we all agree the MAC needs to see a down/up, even if the > link itself never went down. Anything else is too risky and will > probably break something somewhere. Means, this patch should be dropped. Are there other changes required? Regards, Oleksij
On Tue, Feb 27, 2024 at 06:35:38AM +0100, Oleksij Rempel wrote: > On Mon, Feb 26, 2024 at 07:59:28PM +0100, Andrew Lunn wrote: > > On Mon, Feb 26, 2024 at 05:53:31PM +0000, Russell King (Oracle) wrote: > > > On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote: > > > > This is the source of the concern, we don't know which MAC drivers we might > > > > end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully > > > > none, because they should be well written to only update the parameters that > > > > need updating, but who knows? > > > > > > Just quickly... There are some (I went through a bunch.) They don't > > > support EEE. I haven't been through all though, so there could be > > > some which support EEE and where adjust_link() with phydev->link=true > > > twice in a row could result in badness. > > > > So i think we all agree the MAC needs to see a down/up, even if the > > link itself never went down. Anything else is too risky and will > > probably break something somewhere. > > Means, this patch should be dropped. No. This patch handles the case that EEE is changed, but does not require an auto-neg cycle. If you drop this patch, that use case breaks. You need to extend this patch to signal to the MAC a down followed by an up. It is a fake down, the media side never goes down, but the MAC needs to think it has in order to keep with the usual convention that we never call adjust_link() twice with phydev->link not changing. Andrew
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index fa5145c9328e..f98600ed3b35 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -1550,6 +1550,8 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee); * advertised, but the previously advertised link modes are * retained. This allows EEE to be enabled/disabled in a * non-destructive way. + * Returns either error code, 0 if there was no change, or positive + * value if there was a change which triggered auto-neg. */ int genphy_c45_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data) @@ -1581,9 +1583,12 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev, ret = genphy_c45_an_config_eee_aneg(phydev); if (ret < 0) return ret; - if (ret > 0) - return phy_restart_aneg(phydev); - + if (ret > 0) { + ret = phy_restart_aneg(phydev); + if (ret < 0) + return ret; + return 1; + } return 0; } EXPORT_SYMBOL(genphy_c45_ethtool_set_eee); diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index f0ed07c74a36..9e26508d5a31 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -988,7 +988,8 @@ static int phy_check_link_status(struct phy_device *phydev) if (err < 0) phydev->enable_tx_lpi = false; else - phydev->enable_tx_lpi = !!err; + phydev->enable_tx_lpi = (err & phydev->eee_cfg.tx_lpi_enabled); + phy_link_up(phydev); } else if (!phydev->link && phydev->state != PHY_NOLINK) { phydev->state = PHY_NOLINK; @@ -1679,6 +1680,21 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data) } EXPORT_SYMBOL(phy_ethtool_get_eee); +/* auto-neg not triggered, directly inform the MAC if something + * changed + */ +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, + struct ethtool_keee *data) +{ + if (phydev->eee_cfg.tx_lpi_enabled != + data->tx_lpi_enabled) { + eee_to_eeecfg(data, &phydev->eee_cfg); + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); + if (phydev->link) + phy_link_up(phydev); + } +} + /** * phy_ethtool_set_eee - set EEE supported and status * @phydev: target phy_device struct @@ -1695,11 +1711,14 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data) mutex_lock(&phydev->lock); ret = genphy_c45_ethtool_set_eee(phydev, data); - if (!ret) + if (ret >= 0) { + if (ret == 0) + phy_ethtool_set_eee_noneg(phydev, data); eee_to_eeecfg(data, &phydev->eee_cfg); + } mutex_unlock(&phydev->lock); - return ret; + return ret < 0 ? ret : 0; } EXPORT_SYMBOL(phy_ethtool_set_eee);