netdev - Re: [PATCH net-next v5] Add Mellanox BlueField Gigabit Ethernet driver

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:   Tue, 1 Jun 2021 15:19:51 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Asmaa Mnebhi <asmaa@...dia.com>
Cc:     David Thompson <davthompson@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Liming Sun <limings@...dia.com>
Subject: Re: [PATCH net-next v5] Add Mellanox BlueField Gigabit Ethernet
 driver

Please do not top post.

> Thanks.
> Asmaa
> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch> 
> Sent: Friday, May 28, 2021 8:22 PM
> To: David Thompson <davthompson@...dia.com>
> Cc: davem@...emloft.net; kuba@...nel.org; netdev@...r.kernel.org; Liming Sun <limings@...dia.com>; Asmaa Mnebhi <asmaa@...dia.com>
> Subject: Re: [PATCH net-next v5] Add Mellanox BlueField Gigabit Ethernet driver
> 
> > +static void mlxbf_gige_adjust_link (struct net_device *netdev) {
> > +	struct mlxbf_gige *priv = netdev_priv(netdev);
> > +	struct phy_device *phydev = netdev->phydev;
> > +
> > +	if (phydev->link) {
> > +		priv->rx_pause = phydev->pause;
> > +		priv->tx_pause = phydev->pause;
> > +	}
> 
> ...
> 
> > +	/* MAC supports symmetric flow control */
> > +	phy_support_sym_pause(phydev);
> 

> What i don't see anywhere is you acting on the results of the pause
> negotiation. It could be, mlxbf_gige_adjust_link() tells you the
> peer does not support pause, and you need to disable it in this MAC
> as well. It is a negotiation, after all.

>From what you are saying, this is all wrong. You don't negotiate at
all. So don't report negotiated values in ethtool, just report the
fixed values, and do not set autoneg in ethtool_pauseparam because you
have not negotiated it.

You also should not be calling phy_support_sym_pause(), which means, i
support negotiated pause up to and including symmetric pause. You
might also need to clear the pause bits from phydev->advertising.

	Andrew