netdev - Re: [PATCH net] net: spacemit: Implement emac_set

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: <c180925d-68fe-4af1-aa4f-57fb2cd1e9ca@lunn.ch>
Date: Fri, 31 Oct 2025 13:43:44 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Vivian Wang <wangruikang@...as.ac.cn>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Yixun Lan <dlan@...too.org>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Troy Mitchell <troy.mitchell@...ux.spacemit.com>,
	netdev@...r.kernel.org, linux-riscv@...ts.infradead.org,
	spacemit@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

On Fri, Oct 31, 2025 at 03:22:56PM +0800, Vivian Wang wrote:
> 
> On 10/31/25 05:32, Andrew Lunn wrote:
> >> [...]
> >>
> >> -		emac_set_fc(priv, fc);
> >> -	}
> >> +	phy_set_asym_pause(dev->phydev, pause->rx_pause, pause->tx_pause);
> > It is hard to read what this patch is doing, but there are 3 use cases.
> 
> Yeah, I guess the patch doesn't look great. I'll reorganize it in the
> next version to make it clearer what the new implementation is and also
> fix it up per your other comments.
> 
> > 1) general autoneg for link speed etc, and pause autoneg
> > 2) general autoneg for link speed etc, and forced pause
> > 3) forced link speed etc, and forced pause.
> 
> Thanks for the tip on the different cases. However, there's one bit I
> don't really understand: Isn't this set_pauseparam thing only for
> setting pause autoneg / force?

Nope. You need to think about how it interacts with generic autoneg.

       ethtool -A|--pause devname [autoneg on|off] [rx on|off] [tx on|off]

       ethtool -s devname [speed N] [lanes N] [duplex half|full]
              [port tp|aui|bnc|mii] [mdix auto|on|off] [autoneg on|off]

These autoneg are different things. -s is about generic autoneg,
speed, duplex, etc. However pause can also be negotiated, or not,
using -A.

You can only autoneg pause if you are doing generic autoneg. So there
are three combinations you need to handle.

With pause autoneg off, you can set registers in the MAC immediately,
but you need to be careful not to overwrite the values when generic
autoneg completes and the adjust_link callback is called.

If you have pause autoneg on, you have to wait for the adjust_link
callback to be called with the results of the negotiation, including
pause.

phylink hides all this logic. There is a link_up callback, which tells
you how to program the hardware. You just do it, no logic needed.

	Andrew