diff mbox series

[net-next,v2] net: phy: dp83826: disable WOL at init

Message ID 20240226162339.696461-1-catalin.popescu@leica-geosystems.com
State New
Headers
Series [net-next,v2] net: phy: dp83826: disable WOL at init |

Commit Message

POPESCU Catalin Feb. 26, 2024, 4:23 p.m. UTC 
  Commit d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
introduced a regression in that WOL is not disabled by default for DP83826.
WOL should normally be enabled through ethtool.

Fixes: d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
Changes in v2:
 - add Fixes tag to commit message
 - update subject prefix to [PATCH net-next]
---
 drivers/net/phy/dp83822.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 33e1d31873f87d119e5120b88cd350efa68ef276
prerequisite-patch-id: 0000000000000000000000000000000000000000

Comments

Russell King (Oracle) Feb. 26, 2024, 4:37 p.m. UTC | #1 
On Mon, Feb 26, 2024 at 05:23:39PM +0100, Catalin Popescu wrote:
> Commit d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
> introduced a regression in that WOL is not disabled by default for DP83826.
> WOL should normally be enabled through ethtool.
> 
> Fixes: d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>

It seems rather interesting that WoL is disabled in the config_init()
method - because this will be called when the PHY is attached to its
network driver (reasonable I guess) but also at resume time - which
means one can't just set the WoL mode once and that status will be
preserved.

Maybe Andrew can clarify, but I thought once WoL was configured, that
configuration should remain until the system is rebooted.
Andrew Lunn Feb. 26, 2024, 4:53 p.m. UTC | #2 
On Mon, Feb 26, 2024 at 04:37:55PM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 26, 2024 at 05:23:39PM +0100, Catalin Popescu wrote:
> > Commit d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
> > introduced a regression in that WOL is not disabled by default for DP83826.
> > WOL should normally be enabled through ethtool.
> > 
> > Fixes: d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
> > Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> 
> It seems rather interesting that WoL is disabled in the config_init()
> method - because this will be called when the PHY is attached to its
> network driver (reasonable I guess) but also at resume time - which
> means one can't just set the WoL mode once and that status will be
> preserved.
> 
> Maybe Andrew can clarify, but I thought once WoL was configured, that
> configuration should remain until the system is rebooted.

Yes, i noticed this as well.

d1d77120bc28 does change the behaviour, clearing WoL was dropped. As
you say, WoL was probably broken before d1d77120bc28 and it will still
be broken after this patch, so no real change there :-)

Catalin as a followup, could you please look at suspend/resume via
WoL. As Russell pointed out, if the machine is woken from suspend, we
expect the WoL settings to be kept. So WoL should be cleared on boot,
but not resume.

    Andrew
POPESCU Catalin Feb. 27, 2024, 8:03 a.m. UTC | #3 
On 26.02.24 17:53, Andrew Lunn wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Mon, Feb 26, 2024 at 04:37:55PM +0000, Russell King (Oracle) wrote:
>> On Mon, Feb 26, 2024 at 05:23:39PM +0100, Catalin Popescu wrote:
>>> Commit d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
>>> introduced a regression in that WOL is not disabled by default for DP83826.
>>> WOL should normally be enabled through ethtool.
>>>
>>> Fixes: d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>> It seems rather interesting that WoL is disabled in the config_init()
>> method - because this will be called when the PHY is attached to its
>> network driver (reasonable I guess) but also at resume time - which
>> means one can't just set the WoL mode once and that status will be
>> preserved.
>>
>> Maybe Andrew can clarify, but I thought once WoL was configured, that
>> configuration should remain until the system is rebooted.
> Yes, i noticed this as well.
>
> d1d77120bc28 does change the behaviour, clearing WoL was dropped. As
> you say, WoL was probably broken before d1d77120bc28 and it will still
> be broken after this patch, so no real change there :-)
>
> Catalin as a followup, could you please look at suspend/resume via
> WoL. As Russell pointed out, if the machine is woken from suspend, we
> expect the WoL settings to be kept. So WoL should be cleared on boot,
> but not resume.
Sure, I will look into it. I agree that suspend should not change the 
configuration.
>
>      Andrew
patchwork-bot+netdevbpf@kernel.org Feb. 29, 2024, 3:50 a.m. UTC | #4 
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 26 Feb 2024 17:23:39 +0100 you wrote:
> Commit d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
> introduced a regression in that WOL is not disabled by default for DP83826.
> WOL should normally be enabled through ethtool.
> 
> Fixes: d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net: phy: dp83826: disable WOL at init
    https://git.kernel.org/netdev/net-next/c/e83ddcea6549

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index 30f2616ab1c2..ba320dc3df98 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -528,7 +528,7 @@  static int dp83826_config_init(struct phy_device *phydev)
 			return ret;
 	}
 
-	return 0;
+	return dp8382x_disable_wol(phydev);
 }
 
 static int dp8382x_config_init(struct phy_device *phydev)