| Message ID | E1tXhVK-000n18-3C@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add phylink managed EEE support | expand |
On Tue, Jan 14, 2025 at 02:02:50PM +0000, Russell King (Oracle) wrote: > In order to allow DSA drivers to use phylink managed EEE, changes are > necessary to the DSA .set_eee() and .get_eee() methods. Where drivers > make use of phylink managed EEE, these should just pass the method on > to their phylink implementation without calling the DSA specific > operations. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- What is the reason for including this patch with this set, where it is of no use until at least one DSA driver provides the new API implementations? > net/dsa/user.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/net/dsa/user.c b/net/dsa/user.c > index c74f2b2b92de..6912d2d57486 100644 > --- a/net/dsa/user.c > +++ b/net/dsa/user.c > @@ -1233,16 +1233,23 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e) > if (!ds->ops->support_eee || !ds->ops->support_eee(ds, dp->index)) > return -EOPNOTSUPP; > > - /* Port's PHY and MAC both need to be EEE capable */ > - if (!dev->phydev) > - return -ENODEV; > - > - if (!ds->ops->set_mac_eee) > - return -EOPNOTSUPP; > + /* If the port is using phylink managed EEE, then get_mac_eee is set_mac_eee() is what is unnecessary. > + * unnecessary. > + */ > + if (!ds->phylink_mac_ops || > + !ds->phylink_mac_ops->mac_disable_tx_lpi || > + !ds->phylink_mac_ops->mac_enable_tx_lpi) { Does it make sense to export pl->mac_supports_eee_ops wrapped into a helper function and call that here? To avoid making DSA too tightly coupled with the phylink MAC operation names. > + /* Port's PHY and MAC both need to be EEE capable */ > + if (!dev->phydev) > + return -ENODEV; > + > + if (!ds->ops->set_mac_eee) > + return -EOPNOTSUPP; > > - ret = ds->ops->set_mac_eee(ds, dp->index, e); > - if (ret) > - return ret; > + ret = ds->ops->set_mac_eee(ds, dp->index, e); > + if (ret) > + return ret; > + } > > return phylink_ethtool_set_eee(dp->pl, e); > } > -- > 2.30.2 >
On Tue, Jan 14, 2025 at 09:26:56PM +0200, Vladimir Oltean wrote: > On Tue, Jan 14, 2025 at 02:02:50PM +0000, Russell King (Oracle) wrote: > > In order to allow DSA drivers to use phylink managed EEE, changes are > > necessary to the DSA .set_eee() and .get_eee() methods. Where drivers > > make use of phylink managed EEE, these should just pass the method on > > to their phylink implementation without calling the DSA specific > > operations. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > What is the reason for including this patch with this set, where > it is of no use until at least one DSA driver provides the new API > implementations? No criticism against you, but I guess you didn't read the cover message? I tend not to read cover messages. I've been wondering for a while now whether anyone actually does and thus whether they are worth the effort of writing anything beyond providing a message ID to tie a series together and a diffstat for the series. Here's the relevant bit: "The remainder of the patches convert mvneta, lan743x and stmmac, add support for mvneta, and add the basics that will be necessary into the DSA code for DSA drivers to make use of this. "I would like to get patches 1 through 9 into net-next before the merge window, but we're running out of time for that." So, it's included in this RFC series not because I'm intending it to be merged, but so that people can see what DSA requires to make it functional there, and provide review comments if they see fit - which you have done, thanks. I'm sure if I'd said "I have patches for DSA" you'd have responded asking to see them! Of course, I do have changes that will require this - mt753x - but that patch is not quite ready because this series I've posted has seen a few changes recently to remove stuff that was never settled (the LPI timer questions, whether it should be validated, clamped, should phylink provide a software timer if the LPI timer is out of range, etc.) That's more proof that text in cover messages is utterly useless! > > net/dsa/user.c | 25 ++++++++++++++++--------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/net/dsa/user.c b/net/dsa/user.c > > index c74f2b2b92de..6912d2d57486 100644 > > --- a/net/dsa/user.c > > +++ b/net/dsa/user.c > > @@ -1233,16 +1233,23 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e) > > if (!ds->ops->support_eee || !ds->ops->support_eee(ds, dp->index)) > > return -EOPNOTSUPP; > > > > - /* Port's PHY and MAC both need to be EEE capable */ > > - if (!dev->phydev) > > - return -ENODEV; > > - > > - if (!ds->ops->set_mac_eee) > > - return -EOPNOTSUPP; > > + /* If the port is using phylink managed EEE, then get_mac_eee is > > set_mac_eee() is what is unnecessary. Thanks for spotting that. > > + * unnecessary. > > + */ > > + if (!ds->phylink_mac_ops || > > + !ds->phylink_mac_ops->mac_disable_tx_lpi || > > + !ds->phylink_mac_ops->mac_enable_tx_lpi) { > > Does it make sense to export pl->mac_supports_eee_ops wrapped into a > helper function and call that here? To avoid making DSA too tightly > coupled with the phylink MAC operation names. I could wrap the test up in an inline function which would avoid the duplication. Thanks for the suggestion.
On Tue, Jan 14, 2025 at 08:02:55PM +0000, Russell King (Oracle) wrote: > > What is the reason for including this patch with this set, where > > it is of no use until at least one DSA driver provides the new API > > implementations? > > No criticism against you, but I guess you didn't read the cover > message? I tend not to read cover messages. I've been wondering for a > while now whether anyone actually does and thus whether they are worth > the effort of writing anything beyond providing a message ID to tie a > series together and a diffstat for the series. > > Here's the relevant bit: > > "The remainder of the patches convert mvneta, lan743x and stmmac, add > support for mvneta, and add the basics that will be necessary into the > DSA code for DSA drivers to make use of this. > > "I would like to get patches 1 through 9 into net-next before the > merge window, but we're running out of time for that." > > So, it's included in this RFC series not because I'm intending it to > be merged, but so that people can see what DSA requires to make it > functional there, and provide review comments if they see fit - which > you have done, thanks. > > I'm sure if I'd said "I have patches for DSA" you'd have responded > asking to see them! > > Of course, I do have changes that will require this - mt753x - but > that patch is not quite ready because this series I've posted has seen > a few changes recently to remove stuff that was never settled (the > LPI timer questions, whether it should be validated, clamped, should > phylink provide a software timer if the LPI timer is out of range, > etc.) That's more proof that text in cover messages is utterly > useless! Ok, for your information, I had read the cover letter in its entirety, including the paragraph which you are highlighting here again. I still find it of limited usefulness for me to see and not be able to leave meaningful feedback on a DSA conversion broken down into sets which offer an incomplete image, especially when the API itself is still subject to change. I wanted to leave an Acked-by but I didn't fully understand where the conversion is going, so I asked the question instead. I wonder what you would have done in my position. I wish you a beautiful evening.
diff --git a/net/dsa/user.c b/net/dsa/user.c index c74f2b2b92de..6912d2d57486 100644 --- a/net/dsa/user.c +++ b/net/dsa/user.c @@ -1233,16 +1233,23 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e) if (!ds->ops->support_eee || !ds->ops->support_eee(ds, dp->index)) return -EOPNOTSUPP; - /* Port's PHY and MAC both need to be EEE capable */ - if (!dev->phydev) - return -ENODEV; - - if (!ds->ops->set_mac_eee) - return -EOPNOTSUPP; + /* If the port is using phylink managed EEE, then get_mac_eee is + * unnecessary. + */ + if (!ds->phylink_mac_ops || + !ds->phylink_mac_ops->mac_disable_tx_lpi || + !ds->phylink_mac_ops->mac_enable_tx_lpi) { + /* Port's PHY and MAC both need to be EEE capable */ + if (!dev->phydev) + return -ENODEV; + + if (!ds->ops->set_mac_eee) + return -EOPNOTSUPP; - ret = ds->ops->set_mac_eee(ds, dp->index, e); - if (ret) - return ret; + ret = ds->ops->set_mac_eee(ds, dp->index, e); + if (ret) + return ret; + } return phylink_ethtool_set_eee(dp->pl, e); }
In order to allow DSA drivers to use phylink managed EEE, changes are necessary to the DSA .set_eee() and .get_eee() methods. Where drivers make use of phylink managed EEE, these should just pass the method on to their phylink implementation without calling the DSA specific operations. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- net/dsa/user.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)