netdev - Re: [PATCH net-next 2/2] net: ethernet: cortina: Implement .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] [day] [month] [year] [list]

Message-ID: <55d915d2-28f0-4ed3-8f60-b43e7f4469ac@lunn.ch>
Date: Thu, 9 May 2024 22:08:45 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Hans Ulli Kroll <ulli.kroll@...glemail.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net: ethernet: cortina: Implement
 .set_pauseparam()

On Thu, May 09, 2024 at 09:48:38AM +0200, Linus Walleij wrote:
> The Cortina Gemini ethernet can very well set up TX or RX
> pausing, so add this functionality to the driver in a
> .set_pauseparam() callback.
> 
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 599de7914122..c732e985055f 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2138,6 +2138,28 @@ static void gmac_get_pauseparam(struct net_device *netdev,
>  	pparam->autoneg = true;
>  }
>  
> +static int gmac_set_pauseparam(struct net_device *netdev,
> +				struct ethtool_pauseparam *pparam)
> +{
> +	struct gemini_ethernet_port *port = netdev_priv(netdev);
> +	union gmac_config0 config0;
> +
> +	config0.bits32 = readl(port->gmac_base + GMAC_CONFIG0);
> +
> +	if (pparam->rx_pause)
> +		config0.bits.rx_fc_en = 1;
> +	if (pparam->tx_pause)
> +		config0.bits.tx_fc_en = 1;
> +	/* We only support autonegotiation */
> +	if (!pparam->autoneg)
> +		return -EINVAL;
> +
> +	writel(config0.bits32, port->gmac_base + GMAC_CONFIG0);

Everybody gets pause wrong. You even say it yourself:

> +	/* We only support autonegotiation */

After connecting to the PHY, you need to call
phy_support_asym_pause(), to let phylib know the MAC support
asymmetric pause. The PHY will then advertise this to the link peer.
Gemini does seem to be doing that already.

When auto-neg is complete, it calls the adjust link callback. Ideally
you then want to call phy_get_pause() which gives you the results of
the negotiation, and so how to configure the MAC. gmac_speed_set()
kind of does this, but it directly reads PHY registers, rather than
asking phylib. It would be nice to update this code.

This ethtool call allows you to change what the device advertises to
the link partner. You should pass this onto phylib using
phy_set_asym_pause(). If something has changed, phylib will kick off a
new auto-neg, and then call the adjust link callback so you can
configure the hardware with the new negotiation results. It is
unlikely that gmac_set_pauseparam() needs to change the hardware
configuration when pparam->autoneg is true.

If pparam->autoneg is false, that means we are not using auto-neg for
pause, and then you can directly program the MAC hardware. But it does
not look like you intend to support this, which is fine.

    Andrew

---
pw-bot: cr