netdev - Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close

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: <154bc00e-cf87-6d8d-c3ad-8c3803104e90@g0hl1n.net>
Date:   Mon, 23 Oct 2017 01:01:46 +0200
From:   Richard Leitner <dev@...l1n.net>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Richard Leitner <dev@...l1n.net>, fugang.duan@....com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Richard Leitner <richard.leitner@...data.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close

On 10/23/2017 12:48 AM, Florian Fainelli wrote> On 10/22/2017 03:30 PM, 
Richard Leitner wrote:
>> On 10/22/2017 08:31 PM, Florian Fainelli wrote:
>>> On 10/22/2017 06:11 AM, Richard Leitner wrote:

...

>> Andrew Lunn suggested to make the PHY driver a clock driver and let it
>> export the refclk... But IMHO that "feels" a bit strange. Due to the
>> fact in our case the clock is generated by the SoC and the PHY is an
>> external chip which only consumes it.
> 
> It depends which *MII mode you use. Unless you are using reverse MII,
> the PHY supplies the RX clock and the MAC supplies the RX clock, so
> solving this for all modes becomes quite tricky. Andrew's suggestion
> makes sense, but considering that the MAC and PHY need to be connected
> to each other, it may be tricky to request the PHY's clock before you
> actually attached to the PHY...
>>
>> But back to this patch: Is it OK the way it fixes the issue?
> 
> Fugang and Andrew probably know this hardware a lot better, I would have
> to look at the code path a bit more to understand if an alternative
> solution is possible. It sounds like your patch could create a power
> consumption regression, so maybe add a check for the PHY ID that is
> problematic by doing something like:
> 
> if (priv->phydev->drv && priv->phydev->drv->phy_id == XXXX_XXXX) where
> XXXX_XXXX is the LAN870 PHY ID (obtained from MII register 2 & 3).

I already had a patch which does this check and triggers a reset of the 
PHY after the refclk was enabled (in fec_enet_clk_enable) in case the 
phy_id matches. This would IMHO avoid all power consumption 
regressions... It was something like:

switch (priv->phydev->drv->phy_id) {
case XXXX:
case XXXX:
	ret = fec_reset_phy(ndev);
	if (ret)
		goto failed_reset;
	if (ndev->phydev) {
		ret = phy_init_hw(ndev->phydev);
		if (ret)
			goto failed_reset;
	}
}

Would that be better?

Basically I'm fine with any kind of solution... It's up to you guys... 
You're the experts ;-)