netdev - Re: [PATCH NET V5 2/2] net: hns: Use 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]

Date:   Fri, 30 Jun 2017 17:14:11 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <davem@...emloft.net>, <f.fainelli@...il.com>,
        <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>, <tremyfr@...il.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

Hi, Andrew

On 2017/6/29 21:56, Andrew Lunn wrote:
>>> You only call dev_close() if the device is running. What if somebody
>>> runs the self test on an interface when it has never been opened? It
>>> looks like you will call phy_resume(). But since it has never been
>>> suspended, you could be in trouble.
>> Here is what I can think of:
>> 1. when the mac driver is first loaded, the phy has a default state. suspended?
> 
> Nope. The PHY will only be suspended when you actually call
> phy_suspend.
> 
>> 2. If user runs the self test after using 'ifconfig ethX down', then I suppose
>> phy is already suspended.
> 
> Assuming the phy has at some point been up, after a down, it should be
> suspended.
> 
> The key thing here is, phy_resume() can only be called after a
> successful phy_suspend(). Those are the power management rules, and
> the expectations of the drivers. Doing a resume without first doing an
> explicit suspend is asking for bad things to happen.
> 
> You are having trouble because you are not using the API for what it
> was intended. Maybe you need to take a step back and look at the
> bigger picture of how self tests are being performed. Why do you need
> the dev_close()/dev_open()? Maybe
> netif_device_detach()/netif_device_attach() would be better?
> How do other drivers do self test?
> 
Basically, when doing a loopback test, we put a skb in the tx ring, and
see if we can receive it in the rx ring. And We can't find other functions
that meets our requirement, except dev_close()/dev_open.

netif_device_detach only stop the stack from sending packet, napi_disable is
needed to stop the stack from receiving packet.
And after phy loopback test, netif_device_attach does not bring back the link.
Is there a way to bring back the link?

We still need to resume the phy after 'ifconfig ethX down'. Also how can I
tell the difference between phy being suspended after 'ifconfig ethX down'
and phy being not suspended after mac driver is first loaded?

It seems that ixgbe_ethtool in mainline kernel also use netif_tx_disable,
napi_disable and other hardware specific method to setup self test.
But newest ixgbe_ethtool code in github also use dev_close to do self test.

Any idea?

Best Regard
Yunsheng Lin