netdev - Re: [PATCH] [v3] net: phy: phy drivers should not set SUPPORTED_[Asym

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: <2c2aaf1d-05a5-d2bc-d04a-79224a4c6b43@gmail.com>
Date:   Tue, 6 Dec 2016 16:36:38 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Timur Tabi <timur@...eaurora.org>,
        David Miller <davem@...emloft.net>, jon.mason@...adcom.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH] [v3] net: phy: phy drivers should not set
 SUPPORTED_[Asym_]Pause

On 12/06/2016 04:27 PM, Timur Tabi wrote:
> Instead of having individual PHY drivers set the SUPPORTED_Pause and
> SUPPORTED_Asym_Pause flags, phylib itself should set those flags,
> unless there is a hardware erratum or other special case.  During
> autonegotiation, the PHYs will determine whether to enable pause
> frame support.
> 
> Pause frames are a feature that is supported by the MAC.  It is the MAC
> that generates the frames and that processes them.  The PHY can only be
> configured to allow them to pass through.
> 
> So the new process is:
> 
> 1) Unless the PHY driver overrides it, phylib sets the SUPPORTED_Pause
> and SUPPORTED_AsymPause bits in phydev->supported.  This indicates that
> the PHY supports pause frames.
> 
> 2) The MAC driver checks phydev->supported before it calls phy_start().
> If (SUPPORTED_Pause | SUPPORTED_AsymPause) is set, then the MAC driver
> sets those bits in phydev->advertising, if it wants to enable pause
> frame support.
> 
> 3) When the link state changes, the MAC driver checks phydev->pause and
> phydev->asym_pause,  If the bits are set, then it enables the corresponding
> features in the MAC.  The algorithm is:
> 
> 	if (phydev->pause)
> 		The MAC should be programmed to receive and honor
>                 pause frames it receives, i.e. enable receive flow control.
> 
> 	if (phydev->pause != phydev->asym_pause)
> 		The MAC should be programmed to transmit pause
> 		frames when needed, i.e. enable transmit flow control.
> 
> Signed-off-by: Timur Tabi <timur@...eaurora.org>
> ---

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 49a1c98..fe36eeb 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1598,6 +1598,27 @@ static int phy_probe(struct device *dev)
>  	of_set_phy_supported(phydev);
>  	phydev->advertising = phydev->supported;
>  
> +	/* The Pause Frame bits indicate that the PHY can support passing
> +	 * pause frames. During autonegotiation, the PHYs will determine if
> +	 * they should allow pause frames to pass.  The MAC driver should then
> +	 * use that result to determine whether to enable flow control via
> +	 * pause frames.
> +	 *
> +	 * Normally, PHY drivers should not set the Pause bits, and instead
> +	 * allow phylib to do that.  However, there may be some situations
> +	 * (e.g. hardware erratum) where the driver wants to set only one
> +	 * of these bits.
> +	 */
> +	if (phydrv->features & (SUPPORTED_Pause | SUPPORTED_Asym_Pause)) {
> +		phydev->supported &= ~(SUPPORTED_Pause | SUPPORTED_Asym_Pause);
> +		phydev->supported |= phydrv->features &
> +				     (SUPPORTED_Pause | SUPPORTED_Asym_Pause);

Is not the & (SUPPORTED_Pause | SUPPORTED_Asym_Pause) redundant here anyway?

> +	} else {
> +		phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;

that part looks good.

> +	}
> +
> +	phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;

but this one basically "undoes" what the if () clause did where we
checked if either, or one of the two bits was already set?
-- 
Florian