netdev - Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

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: <ce4c10b5-c2cf-489d-b096-19b5bcd8c49e@lunn.ch>
Date:   Wed, 28 Jun 2023 15:46:53 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Revanth Kumar Uppala <ruppala@...dia.com>, hkallweit1@...il.com,
        netdev@...r.kernel.org, linux-tegra@...r.kernel.org,
        Narayan Reddy <narayanr@...dia.com>
Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support
 in aquantia PHY

On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> > From: Narayan Reddy <narayanr@...dia.com>
> > 
> > Enable flow control support using pause frames in aquantia phy driver.
> > 
> > Signed-off-by: Narayan Reddy <narayanr@...dia.com>
> > Signed-off-by: Revanth Kumar Uppala <ruppala@...dia.com>
> 
> I think this is over-complex.
> 
> >  #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
> > @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev)
> >  	if (!ret)
> >  		aqr107_chip_info(phydev);
> >  
> > +	/* Advertize flow control */
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);
> > +	linkmode_copy(phydev->advertising, phydev->supported);
> 
> This is the wrong place to be doing this, since pause support depends
> not only on the PHY but also on the MAC. There are phylib interfaces
> that MACs should call so that phylib knows that the MAC supports pause
> frames.
> 
> Secondly, the PHY driver needs to tell phylib that the PHY supports
> pause frames, and that's done through either setting the .features
> member in the PHY driver, or by providing a .get_features
> implementation.
> 
> Configuration of the pause advertisement should already be happening
> through the core phylib code.

I really should do a LPC netdev talk "Everybody gets pause wrong..."

genphy_c45_an_config_aneg() will configure pause advertisement. The
PHY driver does not need to configure it, if the PHY follows the
standard and has the configuration in the correct place. As Russell
said, please check the PHYs ability to advertise pause is being
reported correctly, by .get_features, of the default implementation of
.get_features if that is being used. And then check your MAC driver is
also indicating it supports pause.

	Andrew