Bug 975 - Power DOWN of copper interface not working in e1000 driver
Summary: Power DOWN of copper interface not working in e1000 driver
Status: UNCONFIRMED
Alias: None
Product: DPDK
Classification: Unclassified
Component: ethdev (show other bugs)
Version: 22.03
Hardware: All All
: Normal normal
Target Milestone: ---
Assignee: Haiyue Wang
URL:
Depends on:
Blocks:
 
Reported: 2022-03-23 14:51 CET by Tobias Karlsson
Modified: 2022-03-24 02:43 CET (History)
1 user (show)



Attachments

Description Tobias Karlsson 2022-03-23 14:51:41 CET
A bug crept in via commit 44dddd14059f151f39f7e075b887decfc9a10f11

In function e1000_power_down_phy_copper_base()
  in file drivers/net/e1000/base/e1000_base.c

The following change is needed or the interface will not be brought down properly:

-       if (phy->ops.check_reset_block(hw))
+       if (phy->ops.check_reset_block(hw) == E1000_SUCCESS)
                e1000_power_down_phy_copper(hw);

Please also update the comment above these code lines.
Comment 1 David Marchand 2022-03-23 15:16:14 CET
Haiyue, can you have a look ?
Thanks.
Comment 2 Haiyue Wang 2022-03-23 17:49:06 CET
Thanks Tobias,

I found some reference for the same comments, like in:

STATIC void e1000_power_down_phy_copper_82571(struct e1000_hw *hw)

Change it to below code will work for you ? If yes, please submit a patch.

	/* If the management interface is not enabled, then power down */
	if (!(mac->ops.check_mng_mode(hw) || phy->ops.check_reset_block(hw)))
		e1000_power_down_phy_copper(hw);
Comment 3 Tobias Karlsson 2022-03-23 20:23:11 CET
Hi Haiyue!

Yes, reintroducing that function does also work. However, some previous developer did remove the call to the check_mng_mode() and I assume they did that for some reason. I would therefore be hesitant to propose to reintroduce it.

The details of how this code works is beyond my expertise, so I'd prefer if somebody well familiar with the details proposed the correct solution.

Best regards,
Tobias
Comment 4 Haiyue Wang 2022-03-24 02:26:28 CET
It should be:

if (!(e1000_enable_mng_pass_thru(hw) || phy->ops.check_reset_block(hw)))
Comment 5 Haiyue Wang 2022-03-24 02:43:52 CET
It is missed sync since the internal commit: October 3rd, 2021 6:41pm,

Which is removed by rename the function:

-/**
- * e1000_power_down_phy_copper_82575 - Remove link during PHY power down
- * @hw: pointer to the HW structure
- *
- * In the case of a PHY power down to save power, or to turn off link during a
- * driver unload, or wake on lan is not enabled, remove the link.
- **/
-STATIC void e1000_power_down_phy_copper_82575(struct e1000_hw *hw)
-{
-       struct e1000_phy_info *phy = &hw->phy;
-
-       if (!(phy->ops.check_reset_block))
-               return;
-
-       /* If the management interface is not enabled, then power down */
-       if (!(e1000_enable_mng_pass_thru(hw) || phy->ops.check_reset_block(hw)))
-               e1000_power_down_phy_copper(hw);
-
-       return;
-}
-

Note You need to log in before you can comment on or make changes to this bug.