| Message ID | 7d34ec3f-a2b7-41f5-8f4b-46ee78a76267@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: atlantic: convert EEE handling to use linkmode bitmaps | expand |
On Sat, Feb 03, 2024 at 10:25:44PM +0100, Heiner Kallweit wrote: > Convert EEE handling to use linkmode bitmaps. This prepares for > removing the legacy bitmaps from struct ethtool_keee. > No functional change intended. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > .../ethernet/aquantia/atlantic/aq_ethtool.c | 21 +++++++++---------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c > index 0bd1a0a1a..7cc36517b 100644 > --- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c > +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c > @@ -15,6 +15,7 @@ > #include "aq_macsec.h" > #include "aq_main.h" > > +#include <linux/linkmode.h> > #include <linux/ptp_clock_kernel.h> > > static void aq_ethtool_get_regs(struct net_device *ndev, > @@ -681,20 +682,18 @@ static int aq_ethtool_get_ts_info(struct net_device *ndev, > return 0; > } > > -static u32 eee_mask_to_ethtool_mask(u32 speed) > +static void eee_mask_to_ethtool_mask(unsigned long *mode, u32 speed) > { > - u32 rate = 0; > + linkmode_zero(mode); Some comment as to bnx2x. > static int aq_ethtool_get_eee(struct net_device *ndev, struct ethtool_keee *eee) > @@ -713,14 +712,14 @@ static int aq_ethtool_get_eee(struct net_device *ndev, struct ethtool_keee *eee) > if (err < 0) > return err; > > - eee->supported_u32 = eee_mask_to_ethtool_mask(supported_rates); > + eee_mask_to_ethtool_mask(eee->supported, supported_rates); > > if (aq_nic->aq_nic_cfg.eee_speeds) > - eee->advertised_u32 = eee->supported_u32; > + linkmode_copy(eee->advertised, eee->supported); This is again a correct translation. But the underlying implementation seems wrong. aq_ethtool_set_eee() does not appear to allow the advertisement to be changed, so advertised does equal supported. However aq_ethtool_set_eee() does not validate that the user is not changing what is advertised and returning an error. Lets leave it broken, and see if Aquantia/Marvell care. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 03.02.2024 22:50, Andrew Lunn wrote: > On Sat, Feb 03, 2024 at 10:25:44PM +0100, Heiner Kallweit wrote: >> Convert EEE handling to use linkmode bitmaps. This prepares for >> removing the legacy bitmaps from struct ethtool_keee. >> No functional change intended. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- > [...] > > This is again a correct translation. But the underlying implementation > seems wrong. aq_ethtool_set_eee() does not appear to allow the > advertisement to be changed, so advertised does equal > supported. However aq_ethtool_set_eee() does not validate that the > user is not changing what is advertised and returning an error. Lets > leave it broken, and see if Aquantia/Marvell care. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > This patch was by mistake set to "Changes requested". As Andrew mentions in his review comment, the patch is a mechanical change, and it's fine. It's a different story that the underlying implementation it's broken. This should be fixed separately. Same applies to "bnx2x: convert EEE handling to use linkmode bitmaps" > Andrew
On Wed, 7 Feb 2024 07:52:49 +0100 Heiner Kallweit wrote: > > This is again a correct translation. But the underlying implementation > > seems wrong. aq_ethtool_set_eee() does not appear to allow the > > advertisement to be changed, so advertised does equal > > supported. However aq_ethtool_set_eee() does not validate that the > > user is not changing what is advertised and returning an error. Lets > > leave it broken, and see if Aquantia/Marvell care. > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > This patch was by mistake set to "Changes requested". It's because of the comment about zeroing, not the latter one. Sorry, it's impossible for me to guess whether he meant the tag for v2 or he's fine with the patch as is. Andrew has the powers to change pw state, I'm leaving this to him.
On Wed, Feb 07, 2024 at 07:53:14AM -0800, Jakub Kicinski wrote: > On Wed, 7 Feb 2024 07:52:49 +0100 Heiner Kallweit wrote: > > > This is again a correct translation. But the underlying implementation > > > seems wrong. aq_ethtool_set_eee() does not appear to allow the > > > advertisement to be changed, so advertised does equal > > > supported. However aq_ethtool_set_eee() does not validate that the > > > user is not changing what is advertised and returning an error. Lets > > > leave it broken, and see if Aquantia/Marvell care. > > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > This patch was by mistake set to "Changes requested". > > It's because of the comment about zeroing, not the latter one. > > Sorry, it's impossible for me to guess whether he meant the tag > for v2 or he's fine with the patch as is. Andrew has the powers > to change pw state, I'm leaving this to him. Hi Heiner Sorry, i was not explicitly. The linkmode_zero() is not needed, so it would be good to remove it. Then feel free to add my Reviewed-by: Andrew
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c index 0bd1a0a1a..7cc36517b 100644 --- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c @@ -15,6 +15,7 @@ #include "aq_macsec.h" #include "aq_main.h" +#include <linux/linkmode.h> #include <linux/ptp_clock_kernel.h> static void aq_ethtool_get_regs(struct net_device *ndev, @@ -681,20 +682,18 @@ static int aq_ethtool_get_ts_info(struct net_device *ndev, return 0; } -static u32 eee_mask_to_ethtool_mask(u32 speed) +static void eee_mask_to_ethtool_mask(unsigned long *mode, u32 speed) { - u32 rate = 0; + linkmode_zero(mode); if (speed & AQ_NIC_RATE_EEE_10G) - rate |= SUPPORTED_10000baseT_Full; + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, mode); if (speed & AQ_NIC_RATE_EEE_1G) - rate |= SUPPORTED_1000baseT_Full; + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, mode); if (speed & AQ_NIC_RATE_EEE_100M) - rate |= SUPPORTED_100baseT_Full; - - return rate; + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, mode); } static int aq_ethtool_get_eee(struct net_device *ndev, struct ethtool_keee *eee) @@ -713,14 +712,14 @@ static int aq_ethtool_get_eee(struct net_device *ndev, struct ethtool_keee *eee) if (err < 0) return err; - eee->supported_u32 = eee_mask_to_ethtool_mask(supported_rates); + eee_mask_to_ethtool_mask(eee->supported, supported_rates); if (aq_nic->aq_nic_cfg.eee_speeds) - eee->advertised_u32 = eee->supported_u32; + linkmode_copy(eee->advertised, eee->supported); - eee->lp_advertised_u32 = eee_mask_to_ethtool_mask(rate); + eee_mask_to_ethtool_mask(eee->lp_advertised, rate); - eee->eee_enabled = !!eee->advertised_u32; + eee->eee_enabled = !linkmode_empty(eee->advertised); eee->tx_lpi_enabled = eee->eee_enabled; if ((supported_rates & rate) & AQ_NIC_RATE_EEE_MSK)
Convert EEE handling to use linkmode bitmaps. This prepares for removing the legacy bitmaps from struct ethtool_keee. No functional change intended. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- .../ethernet/aquantia/atlantic/aq_ethtool.c | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-)