linux-kernel - Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self

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:   Thu, 22 Jun 2017 09:41:33 +0800
From:   l00371289 <linyunsheng@...wei.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Florian Fainelli <f.fainelli@...il.com>, <davem@...emloft.net>,
        <huangdaode@...ilicon.com>, <xuwei5@...ilicon.com>,
        <liguozhu@...ilicon.com>, <Yisen.Zhuang@...wei.com>,
        <gabriele.paoloni@...wei.com>, <john.garry@...wei.com>,
        <linuxarm@...wei.com>, <salil.mehta@...wei.com>,
        <lipeng321@...wei.com>, <yankejian@...wei.com>,
        <tremyfr@...il.com>, <xieqianqian@...wei.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test

Hi, Andrew

On 2017/6/21 21:34, Andrew Lunn wrote:
>> drivers/net/ph/marvell.c
>> marvell_set_loopback(struct phy_device *dev, bool enable)
>> {
>> 	/* do some device specific setting */
>> 	........
>>
>> 	return genphy_loopback(dev, enable);
>> }
>>
>> I don't know if this makes sense or not?
> 
> Nope, you want something in phy.c like this. Not compiled, not
> tested....
> 
> int phy_loopback(struct phy_device *dev, bool enable)
> {
> 
> 	int err;
> 
> 	mutex_lock(dev->mutex);
> 
> 	if (enable && dev->loopback_enabled) {
> 	   err = -EBUSY;
> 	   goto out;
> 	}
> 
> 	if (!enable && !dev->loopback_enabled) {
> 	   err = -EINVAL;
> 	   goto out;
> 	}
> 
> 	if (dev->drv->loopback)
> 	   err = dev->drv->loopback(dev, enable);
> 
> 	if (!err)
> 	   dev->loopback_enabled = enable
> 
> 	mutex_unlock(dev->mutex);
> 
> out:
> 	return err;
> }
> EXPORT_SYMBOL(phy_loopback);
> 
> The interesting part is how to handle two attempts to enable loopback.
> Should the MAC driver be responsible for preventing that? Or do it in
> the PHY driver? And what will the MAC driver do if it gets EBUSY?
> Return it to user space?
I don't think returning to user space is an option, because it mean
ethtool phy self test would fail because if availability of some software
resoure, which is not some application would expect. here is what I can
think of:

driver/net/ethernet/hisilicon/hns/hns_ethtool.c
int phy_loopback_selftest(struct phy_device *dev)
{
	int err;

	mutex_lock(dev->mutex);

	if (dev->drv->loopback)
		err = dev->drv->loopback(dev, 1);

	if (err)
		goto out;
	
	/* mac driver do the test*/
	.................


	if (dev->drv->loopback)
		err = dev->drv->loopback(dev, 0);

	if (err)
		goto out;

out:
	mutex_unlock(dev->mutex);
	return err;
}

One question I can think of is that, how do we ensure mac driver enable and
disable phy loopback in pair, enable loopback after dev->mutex is taken,
disable loopback  before dev->mutex is released?
I hope I am not missing something obvious, any suggestion and idea?

Best Regards
Yunsheng Lin