[PATCH net-next] net: atlantic: convert EEE handling to use linkmode bitmaps

[PATCH net-next] net: atlantic: convert EEE handling to use linkmode bitmaps

[Heiner Kallweit]

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);
 
 	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)
-- 
2.43.0

Re: [PATCH net-next] net: atlantic: convert EEE handling to use linkmode bitmaps

[Andrew Lunn]

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

Re: [PATCH net-next] net: atlantic: convert EEE handling to use linkmode bitmaps

[Heiner Kallweit]

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

Re: [PATCH net-next] net: atlantic: convert EEE handling to use linkmode bitmaps

[Jakub Kicinski]

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.

Re: [PATCH net-next] net: atlantic: convert EEE handling to use linkmode bitmaps

[Andrew Lunn]

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