| Message ID | 20230217034230.1249661-1-andrew@lunn.ch (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rework MAC drivers EEE support | expand |
On Fri, Feb 17, 2023 at 04:42:12AM +0100, Andrew Lunn wrote: > phy_init_eee() is supposed to be called once auto-neg has been > completed to determine if EEE should be used with the current link > mode. The MAC hardware should then be configured to either enable or > disable EEE. Many drivers get this wrong, calling phy_init_eee() once, > or only in the ethtool set_eee callback. > > This patchset changes the API, such that EEE becomes the same as other > parameters which are determined by auto-neg. As will speed and duplex, > active EEE is now indicated in the phydev structure, and the > adjust_link callbacks have been modified to act upon its value. > > eee_set and eee_get have been simplified, given that the phylib > functions act upon most of the data in eee_set, and fill in most of > the information needed for eee_set. This is a very nice cleanup, and removes a bunch of logic from MAC drivers into the phylib core code that should result in more uniform behaviour across MAC drivers for this feature. Great! I'm left wondering about the phylink using drivers, whether we could go a little further, because there's also the tx_lpi_enabled flag which should also gate whether EEE is enabled at the MAC - and whether that logic could be handled entirely within phylink too. That would mean instead of mac_link_up() being passed the phydev (and EEE is the reason the phydev is passed) we could instead just pass an "eee" flag to tell the MAC to program itself appropriately. Then, the only thing which MAC drivers need to concern themselves with is setting the TX LPI timer to the appropriate value (which may need to happen in mac_link_up()). However, for this series, it's definitely a much needed improvement! Thanks!
> This is a very nice cleanup, and removes a bunch of logic from MAC > drivers into the phylib core code that should result in more > uniform behaviour across MAC drivers for this feature. Great! > > I'm left wondering about the phylink using drivers, whether we could > go a little further, because there's also the tx_lpi_enabled flag > which should also gate whether EEE is enabled at the MAC tx_lpi_enabled is something which i think needs further cleanup. Most MAC drivers ignore it. I added support to a couple of drivers, when it was simple to do. But not all. I'm actually thinking of moving it into phylib. The MAC driver really does not need to care. All it needs is eee_active in the adjust_link callback. I'm currently undecided if to make such a change as part of this patchset, or do it as a follow up. Andrew
On Fri, Feb 17, 2023 at 04:42:12AM +0100, Andrew Lunn wrote: > phy_init_eee() is supposed to be called once auto-neg has been > completed to determine if EEE should be used with the current link > mode. The MAC hardware should then be configured to either enable or > disable EEE. Many drivers get this wrong, calling phy_init_eee() once, > or only in the ethtool set_eee callback. Looking at some of the recent EEE changes (not related to this patch set) I've come across: commit 9b01c885be364526d8c05794f8358b3e563b7ff8 Author: Oleksij Rempel <linux@rempel-privat.de> Date: Sat Feb 11 08:41:10 2023 +0100 net: phy: c22: migrate to genphy_c45_write_eee_adv() This part of the patch is wrong: __genphy_config_aneg(): - if (genphy_config_eee_advert(phydev)) + err = genphy_c45_write_eee_adv(phydev, phydev->supported_eee); The problem here is that these are not equivalent. genphy_config_eee_advert() only clears the broken EEE modes in the advertisement, it doesn't actually set the advertisement to anything in particular. The replacement code _configures_ the advertisement to whatever the second argument is, which means each time the advertisement is changed (and thus __genphy_config_aneg() is called) the EEE advertisement will ignore whatever the user configured via the set_eee() APIs, and be restored to the full EEE capabilities in the supported mask. This is an obvious regression that needs fixing, especially as the merge window is potentially due to open this weekend. Moreover, it looks like Oleksij's patch was not Cc'd to me (I can't find it in my mailbox) and as I'm listed in MAINTAINERS for phylib, this should have been brought up _before_ Oleksij's patch was applied to net-next (despite me being unlikely to reply to it due to covid, it still would be nice to have reviewed it, or even reply to the damn patch about these concerns.) But I'm having to pick some other damn series to bring up this concern.
On Fri, Feb 17, 2023 at 02:25:57PM +0000, Russell King (Oracle) wrote: > On Fri, Feb 17, 2023 at 04:42:12AM +0100, Andrew Lunn wrote: > > phy_init_eee() is supposed to be called once auto-neg has been > > completed to determine if EEE should be used with the current link > > mode. The MAC hardware should then be configured to either enable or > > disable EEE. Many drivers get this wrong, calling phy_init_eee() once, > > or only in the ethtool set_eee callback. > > Looking at some of the recent EEE changes (not related to this patch > set) I've come across: > > commit 9b01c885be364526d8c05794f8358b3e563b7ff8 > Author: Oleksij Rempel <linux@rempel-privat.de> > Date: Sat Feb 11 08:41:10 2023 +0100 > > net: phy: c22: migrate to genphy_c45_write_eee_adv() > > This part of the patch is wrong: > > __genphy_config_aneg(): > - if (genphy_config_eee_advert(phydev)) > + err = genphy_c45_write_eee_adv(phydev, phydev->supported_eee); > > The problem here is that these are not equivalent. > > genphy_config_eee_advert() only clears the broken EEE modes in the > advertisement, it doesn't actually set the advertisement to anything > in particular. > > The replacement code _configures_ the advertisement to whatever the > second argument is, which means each time the advertisement is > changed (and thus __genphy_config_aneg() is called) the EEE > advertisement will ignore whatever the user configured via the > set_eee() APIs, and be restored to the full EEE capabilities in the > supported mask. > > This is an obvious regression that needs fixing, especially as the > merge window is potentially due to open this weekend. You are right :( I'll be able to come with a fix this Monday. Regards, Oleksij