Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Oct 2021 19:23:15 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Guangbin Huang <huangguangbin2@huawei.com>
Cc:     davem@davemloft.net, kuba@kernel.org, wangjie125@huawei.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        lipeng321@huawei.com, chenhao288@hisilicon.com
Subject: Re: [PATCH net 1/7] net: hns3: fix pause config problem after
 autoneg disabled
Message-ID: <YXmLA4AbY83UV00f@lunn.ch>
References: <20211027121149.45897-1-huangguangbin2@huawei.com>
 <20211027121149.45897-2-huangguangbin2@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211027121149.45897-2-huangguangbin2@huawei.com>
Precedence: bulk

On Wed, Oct 27, 2021 at 08:11:43PM +0800, Guangbin Huang wrote:

The semantics are not too well defined here, the ethtool documentation
is not too clear. Here is how i interpret it.

> If a TP port is configured by follow steps:
> 1.ethtool -s ethx autoneg off speed 100 duplex full

So you turn general autoneg off

> 2.ethtool -A ethx rx on tx on

You did not use autoneg off here. Pause autoneg is separate to general
autoneg. So pause autoneg is still enabled at this point. That means
you should not directly configure the MAC with the pause
configuration, you only do that when pause autoneg is off. You can
consider this as setting how you want pause to be negotiated once
general autoneg is re-enabled.

> 3.ethtool -s ethx autoneg on(rx&tx negotiated pause results are off)

So you reenable general autoneg. As part of that general autoneg,
pause will re-renegotiated, and it should you the preferences you set
in 2, that rx and tx pause can be used. What is actually used depends
on the link peer. The link_adjust callback from phylib tells you how
to program the MAC.

> 4.ethtool -s ethx autoneg off speed 100 duplex full

So you turn general autoneg off again. It is unclear how you are
supposed to program the MAC, but i guess most systems keep with the
result from the last autoneg.

Looking at your patch, there are suspicious calls to phy_syspend and
phy_resume. They don't look correct at all, and i'm not aware of any
other MAC driver doing this. Now, i know the behaviour is not well
defined here, but i'm not sure your interpretation is valid and how
others interpret it.

       Andrew