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

[Andrew Lunn <andrew@lunn.ch>]

Please do not top post.

> Thanks.
> Asmaa
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch> 
> Sent: Friday, May 28, 2021 8:22 PM
> To: David Thompson <davthompson@nvidia.com>
> Cc: davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org; Liming Sun <limings@nvidia.com>; Asmaa Mnebhi <asmaa@nvidia.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