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

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Tue, Nov 19, 2024 at 05:06:33PM +0800, Choong Yong Liang wrote:
> 
> 
> 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.

Hm, the PTP issue with EEE is usually related to PHYs implementing the
EEE without MAC/LPI support. This PHYs are buffering frames and changing
the transmission time, so if the time stamp is made by MAC it will have
different real transmission time. So far i know, Atheros and Realtek
implement it, even if it is not always officially documented, it can be
tested.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |