ixgbe: E610: do not fill EEE lp_advertised from local PHY caps

[PATCH] ixgbe: E610: do not fill EEE lp_advertised from local PHY caps

Posted by David Carlier 1 month, 1 week ago

ixgbe_get_eee_e610() fills kedata->lp_advertised from pcaps.eee_cap
returned by ixgbe_aci_get_phy_caps() with IXGBE_ACI_REPORT_ACTIVE_CFG.
That report mode (and the other IXGBE_ACI_REPORT_* modes) describe the
local PHY only, not the link partner. The X550 path uses a separate
FW_PHY_ACT_UD_2 activity for partner data; the E610 ACI has no
equivalent.

Leave lp_advertised zeroed via the existing linkmode_zero() and drop
the now-unused ixgbe_eee_cap_map[]. eee_active/eee_enabled are
unaffected (sourced from link.eee_status).

Fixes: b61dbdeff3a9 ("ixgbe: E610: add EEE support")
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  | 20 -------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 6990fe53f049..36e43b5e88d1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -3558,17 +3558,6 @@ static const struct {
 	{ FW_PHY_ACT_UD_2_10G_KR_EEE, ETHTOOL_LINK_MODE_10000baseKR_Full_BIT},
 };
 
-static const struct {
-	u16 eee_cap_bit;
-	u32 link_mode;
-} ixgbe_eee_cap_map[] = {
-	{ IXGBE_ACI_PHY_EEE_EN_100BASE_TX, ETHTOOL_LINK_MODE_100baseT_Full_BIT },
-	{ IXGBE_ACI_PHY_EEE_EN_1000BASE_T, ETHTOOL_LINK_MODE_1000baseT_Full_BIT },
-	{ IXGBE_ACI_PHY_EEE_EN_10GBASE_T, ETHTOOL_LINK_MODE_10000baseT_Full_BIT },
-	{ IXGBE_ACI_PHY_EEE_EN_5GBASE_T, ETHTOOL_LINK_MODE_5000baseT_Full_BIT },
-	{ IXGBE_ACI_PHY_EEE_EN_2_5GBASE_T, ETHTOOL_LINK_MODE_2500baseT_Full_BIT },
-};
-
 static int ixgbe_validate_keee(struct net_device *netdev,
 			       struct ethtool_keee *keee_requested)
 {
@@ -3645,7 +3634,6 @@ static int ixgbe_get_eee_e610(struct net_device *netdev,
 	struct ixgbe_aci_cmd_get_phy_caps_data pcaps;
 	struct ixgbe_hw *hw = &adapter->hw;
 	struct ixgbe_link_status link;
-	u16 eee_cap;
 	int err;
 
 	linkmode_zero(kedata->lp_advertised);
@@ -3670,14 +3658,6 @@ static int ixgbe_get_eee_e610(struct net_device *netdev,
 	if (kedata->eee_enabled)
 		kedata->tx_lpi_timer = le16_to_cpu(pcaps.eee_entry_delay);
 
-	eee_cap = le16_to_cpu(pcaps.eee_cap);
-
-	for (int i = 0; i < ARRAY_SIZE(ixgbe_eee_cap_map); i++) {
-		if (eee_cap & ixgbe_eee_cap_map[i].eee_cap_bit)
-			linkmode_set_bit(ixgbe_eee_cap_map[i].link_mode,
-					 kedata->lp_advertised);
-	}
-
 	for (int i = 0; i < ARRAY_SIZE(ixgbe_ls_map); i++) {
 		if (hw->phy.eee_speeds_supported &
 		    ixgbe_ls_map[i].mac_speed)
-- 
2.53.0

Re: [PATCH] ixgbe: E610: do not fill EEE lp_advertised from local PHY caps

Posted by Andrew Lunn 1 month, 1 week ago

On Mon, May 04, 2026 at 07:22:57AM +0100, David Carlier wrote:
> ixgbe_get_eee_e610() fills kedata->lp_advertised from pcaps.eee_cap
> returned by ixgbe_aci_get_phy_caps() with IXGBE_ACI_REPORT_ACTIVE_CFG.
> That report mode (and the other IXGBE_ACI_REPORT_* modes) describe the
> local PHY only, not the link partner. The X550 path uses a separate
> FW_PHY_ACT_UD_2 activity for partner data; the E610 ACI has no
> equivalent.
> 
> Leave lp_advertised zeroed via the existing linkmode_zero() and drop
> the now-unused ixgbe_eee_cap_map[]. eee_active/eee_enabled are
> unaffected (sourced from link.eee_status).

Hi David

Did you test the EEE autoneg capabilities? Is just lp_advertise wrong,
or is the negotiation itself also broken?

	Andrew

Re: [PATCH] ixgbe: E610: do not fill EEE lp_advertised from local PHY caps

Posted by David CARLIER 1 month, 1 week ago

Hi Andrew,

  No E610 here, found it by reading the code - the X550 path
  (ixgbe_get_eee_fw) uses a separate FW_PHY_ACT_UD_2 activity and
  ixgbe_lp_map[] for partner data, the E610 path just feeds
  pcaps.eee_cap from REPORT_ACTIVE_CFG into lp_advertised. None of
  the IXGBE_ACI_REPORT_* modes return partner info so that field
  can't be right.

  The set path goes hw->mac.ops.setup_eee() ->
ixgbe_aci_set_phy_cfg(),
  so negotiation is in the firmware. eee_active / eee_enabled come
  from link.eee_status from the same FW, if those bits are right then
  negotiation works. Can't say more without hardware, Jedrzej or
  Aleksandr would know.

Cheers

Re: [PATCH] ixgbe: E610: do not fill EEE lp_advertised from local PHY caps

Posted by Jacob Keller 1 month, 1 week ago

On 5/4/2026 7:05 AM, David CARLIER wrote:
> Hi Andrew,
> 
>   No E610 here, found it by reading the code - the X550 path
>   (ixgbe_get_eee_fw) uses a separate FW_PHY_ACT_UD_2 activity and
>   ixgbe_lp_map[] for partner data, the E610 path just feeds
>   pcaps.eee_cap from REPORT_ACTIVE_CFG into lp_advertised. None of
>   the IXGBE_ACI_REPORT_* modes return partner info so that field
>   can't be right.
> 
>   The set path goes hw->mac.ops.setup_eee() ->
> ixgbe_aci_set_phy_cfg(),
>   so negotiation is in the firmware. eee_active / eee_enabled come
>   from link.eee_status from the same FW, if those bits are right then
>   negotiation works. Can't say more without hardware, Jedrzej or
>   Aleksandr would know.
> 
> Cheers

Hi David,

Thanks for the report and possible patch. The EEE support just merged,
and I believe the series has undergone testing. It is possible E610 is
significantly different from X550.

@Jedrzej,

Could you please look at this patch and the report from David and
confirm if we need this (or a different?) fix or if the code is correct
for E610 and explain why in that case?

Thanks,
Jake

RE: [PATCH] ixgbe: E610: do not fill EEE lp_advertised from local PHY caps

Posted by Jagielski, Jedrzej 1 month, 1 week ago

>From: Keller, Jacob E <jacob.e.keller@intel.com> 
>Sent: Tuesday, May 5, 2026 12:13 AM
>On 5/4/2026 7:05 AM, David CARLIER wrote:
>> Hi Andrew,
>> 
>>   No E610 here, found it by reading the code - the X550 path
>>   (ixgbe_get_eee_fw) uses a separate FW_PHY_ACT_UD_2 activity and
>>   ixgbe_lp_map[] for partner data, the E610 path just feeds
>>   pcaps.eee_cap from REPORT_ACTIVE_CFG into lp_advertised. None of
>>   the IXGBE_ACI_REPORT_* modes return partner info so that field
>>   can't be right.
>> 
>>   The set path goes hw->mac.ops.setup_eee() ->
>> ixgbe_aci_set_phy_cfg(),
>>   so negotiation is in the firmware. eee_active / eee_enabled come
>>   from link.eee_status from the same FW, if those bits are right then
>>   negotiation works. Can't say more without hardware, Jedrzej or
>>   Aleksandr would know.
>> 
>> Cheers
>
>Hi David,
>
>Thanks for the report and possible patch. The EEE support just merged,
>and I believe the series has undergone testing. It is possible E610 is
>significantly different from X550.
>
>@Jedrzej,
>
>Could you please look at this patch and the report from David and
>confirm if we need this (or a different?) fix or if the code is correct
>for E610 and explain why in that case?
>
>Thanks,
>Jake

Sorry for the delay in responding, i just came back to the office an
 i didn't have access to my mailbox.

After looking into documentation once again and checking it on my setup
i see that David is right. What a catch, thanks! And sorry for my oversight,
i was convinced that negotiated speeds are reported via that field and
it somehow has not been exposed during my tests.

Moreover, looks like E610 currently doesn't report such.

So i believe we would like to have this fix, thank you once again.

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

Jedrek

Re: [PATCH] ixgbe: E610: do not fill EEE lp_advertised from local PHY caps

Posted by Jacob Keller 1 month, 1 week ago

On 5/7/2026 2:50 AM, Jagielski, Jedrzej wrote:
>> From: Keller, Jacob E <jacob.e.keller@intel.com> 
>> Sent: Tuesday, May 5, 2026 12:13 AM
>> On 5/4/2026 7:05 AM, David CARLIER wrote:
>>> Hi Andrew,
>>>
>>>   No E610 here, found it by reading the code - the X550 path
>>>   (ixgbe_get_eee_fw) uses a separate FW_PHY_ACT_UD_2 activity and
>>>   ixgbe_lp_map[] for partner data, the E610 path just feeds
>>>   pcaps.eee_cap from REPORT_ACTIVE_CFG into lp_advertised. None of
>>>   the IXGBE_ACI_REPORT_* modes return partner info so that field
>>>   can't be right.
>>>
>>>   The set path goes hw->mac.ops.setup_eee() ->
>>> ixgbe_aci_set_phy_cfg(),
>>>   so negotiation is in the firmware. eee_active / eee_enabled come
>>>   from link.eee_status from the same FW, if those bits are right then
>>>   negotiation works. Can't say more without hardware, Jedrzej or
>>>   Aleksandr would know.
>>>
>>> Cheers
>>
>> Hi David,
>>
>> Thanks for the report and possible patch. The EEE support just merged,
>> and I believe the series has undergone testing. It is possible E610 is
>> significantly different from X550.
>>
>> @Jedrzej,
>>
>> Could you please look at this patch and the report from David and
>> confirm if we need this (or a different?) fix or if the code is correct
>> for E610 and explain why in that case?
>>
>> Thanks,
>> Jake
> 
> Sorry for the delay in responding, i just came back to the office an
>  i didn't have access to my mailbox.
> 
> After looking into documentation once again and checking it on my setup
> i see that David is right. What a catch, thanks! And sorry for my oversight,
> i was convinced that negotiated speeds are reported via that field and
> it somehow has not been exposed during my tests.
> 
> Moreover, looks like E610 currently doesn't report such.
> 
> So i believe we would like to have this fix, thank you once again.
> 
> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> 
> Jedrek

Great, thanks!