netdev - Re: [PATCH net-next] net: phy: add basic driver for NXP CBTX PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230413135132.ybakyayxflai7tzy@skbuf>
Date:   Thu, 13 Apr 2023 16:51:32 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next] net: phy: add basic driver for NXP CBTX PHY

Hi Heiner,

On Tue, Apr 11, 2023 at 06:49:42PM +0200, Heiner Kallweit wrote:
> > +	return phy_set_bits(phydev, CBTX_PDOWN_CTRL,
> > +			    CBTX_PDOWN_CTL_TRUE_PDOWN);
> 
> A comment may be helpful explaining how true_pdown mode
> is different from power-down mode set by C22 standard
> bit in BMCR as part of genphy_suspend().

The NXP documentation for True Power Down vs General Power Down did not
convince me, and I don't want to speculate and give an answer that might
be incorrect.

There are not many people who can help me give an answer right now
during the holidays. The high level idea is that the PHY may enter a
mode of lower power consumption.

If it's acceptable to you, I can implement suspend and resume as direct
calls to genphy_suspend() and genphy_resume(), and make the change later,
if needed.

> > +static int cbtx_config_intr(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > +		ret = cbtx_ack_interrupts(phydev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = phy_write(phydev, CBTX_IRQ_ENABLE,
> > +				CBTX_IRQ_AN_COMPLETE | CBTX_IRQ_LINK_DOWN);
> 
> I think you need also CBTX_IRQ_ENERGYON. Otherwise you won't get a
> link-up interrupt in forced mode.

I've tested just now with "ethtool -s p1 autoneg off speed 100 duplex full",
and you are exactly correct. I will add the media side energy detection logic
to the enabled IRQ sources in v2. Thanks.