| Message ID | 20230217034230.1249661-4-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rework MAC drivers EEE support | expand |
On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote: > phylib already does most of the work. It will track eee_enabled and > eee_active and correctly set them in the ethtool_get_eee callback. > > Replace the call to phy_init_eee() by looking at the > phydev->eee_active member. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> This is a very nice cleanup, and finally gets rid of that phy_init_eee() in all those mac_link_up() implementations. Yay! Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks!
On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote: > @@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config, > > mvneta_port_up(pp); > > - if (phy && pp->eee_enabled) { > - pp->eee_active = phy_init_eee(phy, false) >= 0; > - mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled); > - } > + if (phy) > + mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled); Thinking about this a bit more, I'm not convinced this is properly safe. What protects phy->eee_active from changing here? The phydev mutex won't be held at this point. As I mentioned in my reply to the cover letter about passing a flag to mac_link_up() for EEE status, this would mean phylink could save the EEE active status just like it does with the other phydev parameters in phylink_phy_change() (which is called under the phydev mutex). That ensures that we grab all the phylib state under the phydev mutex and then use that state when bringing the MAC side up without needing to worry about phydev mutexes. It's way more invasive, as it requires the mac_link_up() method signature to change, but I think it would be a cleaner approach overall.
On Fri, Feb 17, 2023 at 11:59:39AM +0000, Russell King (Oracle) wrote: > On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote: > > @@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config, > > > > mvneta_port_up(pp); > > > > - if (phy && pp->eee_enabled) { > > - pp->eee_active = phy_init_eee(phy, false) >= 0; > > - mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled); > > - } > > + if (phy) > > + mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled); > > Thinking about this a bit more, I'm not convinced this is properly safe. > What protects phy->eee_active from changing here? The phydev mutex won't > be held at this point. > > As I mentioned in my reply to the cover letter about passing a flag to > mac_link_up() for EEE status, this would mean phylink could save the > EEE active status just like it does with the other phydev parameters > in phylink_phy_change() (which is called under the phydev mutex). I suppose another option would be to add a new method to phylink_mac_ops: int (*mac_set_eee)(struct phylink_config *config, bool eee, u32 tx_lpi_timer); and phylink calls this just before mac_link_up() or after phylink_ethtool_set_eee(). The eee flag would be the effective result of phydev->eee_active && tx_lpi_enabled, possibly also && tx_lpi_timer != 0, since a zero tx_lpi_timer is rather meaningless, unless we explicitly have phylink_ethtool_set_eee() reject it is invalid. All that mac_set_eee() implementations should then need to do is to program the LPI timer in the MAC hardware, and enable or disable it according to the "eee" flag. The down-side to another mac_ops method is having to add a wrapper in net/dsa/port.c for it.
On Fri, Feb 17, 2023 at 11:59:39AM +0000, Russell King (Oracle) wrote: > On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote: > > @@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config, > > > > mvneta_port_up(pp); > > > > - if (phy && pp->eee_enabled) { > > - pp->eee_active = phy_init_eee(phy, false) >= 0; > > - mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled); > > - } > > + if (phy) > > + mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled); > > Thinking about this a bit more, I'm not convinced this is properly safe. > What protects phy->eee_active from changing here? The phydev mutex won't > be held at this point. That is one of the differences between phylib and phylink. For phylib, the adjust_link callback is performed while holding phydev lock. So it is guaranteed to be consistent. > It's way more invasive, as it requires the mac_link_up() method > signature to change, but I think it would be a cleaner approach > overall. Yes, i think it has to happen. But i also think it will have a beneficial side effect. Very few MAC drivers implement EEE. Having this parameters will make it much more visible, which could lead to more MAC drivers adding EEE support. Andrew
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 0e39d199ff06..519a08354442 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -536,8 +536,6 @@ struct mvneta_port { struct mvneta_bm_pool *pool_short; int bm_win_id; - bool eee_enabled; - bool eee_active; bool tx_lpi_enabled; u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)]; @@ -4170,7 +4168,6 @@ static void mvneta_mac_link_down(struct phylink_config *config, mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val); } - pp->eee_active = false; mvneta_set_eee(pp, false); } @@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config, mvneta_port_up(pp); - if (phy && pp->eee_enabled) { - pp->eee_active = phy_init_eee(phy, false) >= 0; - mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled); - } + if (phy) + mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled); } static const struct phylink_mac_ops mvneta_phylink_ops = { @@ -5028,8 +5023,6 @@ static int mvneta_ethtool_get_eee(struct net_device *dev, lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0); - eee->eee_enabled = pp->eee_enabled; - eee->eee_active = pp->eee_active; eee->tx_lpi_enabled = pp->tx_lpi_enabled; eee->tx_lpi_timer = (lpi_ctl0) >> 8; // * scale; @@ -5053,7 +5046,6 @@ static int mvneta_ethtool_set_eee(struct net_device *dev, lpi_ctl0 |= eee->tx_lpi_timer << 8; mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi_ctl0); - pp->eee_enabled = eee->eee_enabled; pp->tx_lpi_enabled = eee->tx_lpi_enabled; mvneta_set_eee(pp, eee->tx_lpi_enabled && eee->eee_enabled);
phylib already does most of the work. It will track eee_enabled and eee_active and correctly set them in the ethtool_get_eee callback. Replace the call to phy_init_eee() by looking at the phydev->eee_active member. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/ethernet/marvell/mvneta.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)