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]

Message-ID: <CH2PR12MB38955E99161C89C165D70B76D73E9@CH2PR12MB3895.namprd12.prod.outlook.com>
Date:   Tue, 1 Jun 2021 13:43:58 +0000
From:   Asmaa Mnebhi <asmaa@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
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



-----Original Message-----
From: Andrew Lunn <andrew@...n.ch> 
Sent: Tuesday, June 1, 2021 9:20 AM
To: Asmaa Mnebhi <asmaa@...dia.com>
Cc: David Thompson <davthompson@...dia.com>; davem@...emloft.net; kuba@...nel.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.

Asmaa>> Sounds good! We will update this in our next patch.
I guess I misunderstood. I thought 1G speed always requires autonegotiation. And phy_support_sym_pause would display that the only pause supported by this MAC is symmetric. But what you are saying is that we don't really "negotiate" the pause since our MAC HW supports only symmetric pause?
/**
 * phy_support_sym_pause - Enable support of symmetrical pause
 * @phydev: target phy_device struct
 *
 * Description: Called by the MAC to indicate is supports symmetrical
 * Pause, but not asym pause.
 */
void phy_support_sym_pause(struct phy_device *phydev)