Re: [PATCH net v2 1/2] net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled

From: Choong Yong Liang
Date: Tue Nov 19 2024 - 04:06:50 EST




On 15/11/2024 9:37 pm, Russell King (Oracle) wrote:
On Fri, Nov 15, 2024 at 07:11:50PM +0800, Choong Yong Liang wrote:
Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
designed to have EEE hardware disabled during the initial state.

In the initial stage, phy_probe() sets phydev->eee_enabled to be disabled.
Then, the MAC calls phy_support_eee() to set eee_cfg.eee_enabled to be
enabled. However, when phy_start_aneg() is called,
genphy_c45_an_config_eee_aneg() still refers to phydev->eee_enabled.
This causes the 'ethtool --show-eee' command to show that EEE is enabled,
but in actuality, the driver side is disabled.

This patch will remove phydev->eee_enabled and replace it with
eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(),
it will follow the master configuration to have software and hardware
in sync.

Hmm. I'm not happy with how you're handling my patch. I would've liked
some feedback on it (thanks for spotting that the set_eee case needed
to pass the state to genphy_c45_an_config_eee_aneg()).

However, what's worse is, that the bulk of this patch is my work, yet
you've effectively claimed complete authorship of it in the way you
are submitting this patch. Moreover, you are violating the kernel
submission rules, as the Signed-off-by does not include one for me
(which I need to explicitly give.) I was waiting for the results of
your testing before finalising the patch.

The patch needs to be authored by me, the first sign-off needs to be
me, then optionally Co-developed-by for you, and then your sign-off.

See Documentation/process/submitting-patches.rst

Thanks.

pw-bot: cr


Sorry for the late reply; I just got back from my sick leave. I wasn't aware that you had already submitted a patch. I thought I should include it in my patch series. However, I think I messed up the "Signed-off" part. Sorry about that.

The testing part actually took quite some time to complete, and I was already sick last Friday. I was only able to complete the patch series and resubmit the patch, and I thought we could discuss the test results from the patch series. The issue was initially found with EEE on GPY PHY working together with ptp4l, and it did not meet the expected results. There are many things that need to be tested, as it is not only Marvell PHY that has the issue.

With your patch, most of the issues were resolved based on the testing. However, the set_eee was using the old value of eee_enabled and was not able to turn EEE on or off. I think Heiner's patch already solved that part.

With all the solutions provided, I think the only patch left that I need to submit is the one calling 'phy_support_eee()' from stmmac.