Message-ID: <c1bb831c-fd88-4b03-bda6-d8f4ec4a1681@linux.intel.com>
Date: Tue, 19 Nov 2024 17:06:33 +0800
From: Choong Yong Liang <yong.liang.choong@...ux.intel.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Jose Abreu <joabreu@...opsys.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH net v2 1/2] net: phy: replace phydev->eee_enabled with
 eee_cfg.eee_enabled



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.

Powered by blists - more mailing lists