linux-kernel - Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

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]

Date:   Mon, 20 Nov 2017 10:57:25 +0100
From:   Richard Leitner <richard.leitner@...data.com>
To:     Andy Duan <fugang.duan@....com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     Richard Leitner <dev@...l1n.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
 LAN8710/20


On 11/20/2017 10:47 AM, Andy Duan wrote:
> From: Richard Leitner <dev@...l1n.net> Sent: Monday, November 20, 2017 4:34 PM
>> To: f.fainelli@...il.com; Andy Duan <fugang.duan@....com>;
>> andrew@...n.ch
>> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
>> richard.leitner@...data.com
>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>> LAN8710/20
>>
>> From: Richard Leitner <richard.leitner@...data.com>
>>
>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>> the refclk on and off again during operation (according to their datasheet).
>> Nonetheless exactly this behaviour was introduced for power saving reasons
>> by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>> power").
>> Therefore after enabling the refclk we detect if an affected PHY is attached. If
>> so reset and initialize it again.

...

>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>> +*ndev) {
>> +	struct phy_device *phy_dev = ndev->phydev;
>> +	u32 real_phy_id;
>> +	int ret;
>> +
>> +	/* some PHYs need a reset after the refclk was enabled, so we
>> +	 * reset them here
>> +	 */
>> +	if (!phy_dev)
>> +		return 0;
>> +	if (!phy_dev->drv)
>> +		return 0;
>> +	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>> +	switch (real_phy_id) {
>> +	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
> 
> Don't hard code here...
> I believe there have many other phys also do such operation, hardcode is unacceptable...
> 
> And these code can be put into phy_device.c as common interface.

Ok. Thank you for the feedback.
So it would be fine to hardcode the affected phy_id's in a common
function in phy_device.c?


Another possible solution that came to my mind is to add a flag called
something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
phy_driver. This flag could then be set in the smsc PHY driver for
affected PHYs.

Then instead of comparing the phy_id in the MAC driver this flag could
be checked:

if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
    ret = fec_reset_phy(ndev);
    ...
}

Would checking the flag be OK in fec_main.c?

What would be the "better" approach?

> 
>> +		ret = fec_reset_phy(ndev);
>> +		if (ret)
>> +			return ret;
>> +		ret = phy_init_hw(phy_dev);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>> static int fec_enet_clk_enable(struct net_device *ndev, bool enable)  {
>> 	struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6
>> +1889,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool
>> enable)
>> 		ret = clk_prepare_enable(fep->clk_ref);
>> 		if (ret)
>> 			goto failed_clk_ref;
>> +
>> +		ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>> +		if (ret)
>> +			netdev_warn(ndev, "Resetting PHY failed, connection
>> may be
>> +unstable\n");
>> 	} else {
>> 		clk_disable_unprepare(fep->clk_ahb);
>> 		clk_disable_unprepare(fep->clk_enet_out);
>> @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
>> 	if (ret)
>> 		goto err_enet_mii_probe;
>>
>> +	/* as the PHY is connected now, trigger the reset quirk again */
>> +	ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>> +	if (ret)
>> +		netdev_warn(ndev, "Resetting PHY failed, connection may be
>> +unstable\n");
>> +
>> 	if (fep->quirks & FEC_QUIRK_ERR006687)
>> 		imx6q_cpuidle_fec_irqs_used();
>>
>> 	napi_enable(&fep->napi);
>> 	phy_start(ndev->phydev);
>> +
> 
> No need blank line here...
>> 	netif_tx_start_all_queues(ndev);
>>
>> 	device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
>> --
>> 2.11.0