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:   Wed, 13 Feb 2019 14:12:06 +0100
From:   Niklas Cassel <niklas.cassel@...aro.org>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     David S Miller <davem@...emloft.net>,
        linux-arm-msm@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "Nori, Sekhar" <nsekhar@...com>,
        Peter Ujfalusi <peter.ujfalusi@...com>,
        Marc Gonzalez <marc.w.gonzalez@...e.fr>
Subject: Re: [PATCH] net: phy: at803x: disable delay only for RGMII mode

On Tue, Feb 12, 2019 at 07:49:22PM +0530, Vinod Koul wrote:
> Per "Documentation/devicetree/bindings/net/ethernet.txt" RGMII mode
> should not have delay in PHY whereas RGMII_ID and RGMII_RXID/RGMII_TXID
> can have delay in phy.
> 
> So disable the delay only for RGMII mode and disable for other modes
> 
> Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")
> Reported-by: Peter Ujfalusi <peter.ujfalusi@...com>
> Signed-off-by: Vinod Koul <vkoul@...nel.org>
> ---
>  drivers/net/phy/at803x.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 8ff12938ab47..7b54b54e3316 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -110,6 +110,18 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
>  	return phy_write(phydev, AT803X_DEBUG_DATA, val);
>  }
>  
> +static inline int at803x_enable_rx_delay(struct phy_device *phydev)
> +{
> +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
> +				     AT803X_DEBUG_RX_CLK_DLY_EN);
> +}
> +
> +static inline int at803x_enable_tx_delay(struct phy_device *phydev)
> +{
> +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
> +				     AT803X_DEBUG_TX_CLK_DLY_EN);
> +}
> +
>  static inline int at803x_disable_rx_delay(struct phy_device *phydev)
>  {
>  	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> @@ -255,18 +267,25 @@ static int at803x_config_init(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> -			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
>  		ret = at803x_disable_rx_delay(phydev);
>  		if (ret < 0)
>  			return ret;
> +		ret = at803x_disable_tx_delay(phydev);
> +		if (ret < 0)
> +			return ret;
> +	};
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> +		ret = at803x_enable_rx_delay(phydev);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
> -	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> -			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> -		ret = at803x_disable_tx_delay(phydev);
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> +		ret = at803x_enable_tx_delay(phydev);
>  		if (ret < 0)
>  			return ret;
>  	}

So we have these modes:

PHY_INTERFACE_MODE_RGMII: TX and RX delays disabled
PHY_INTERFACE_MODE_RGMII_ID: TX and RX delays enabled
PHY_INTERFACE_MODE_RGMII_RXID: RX delay enabled, TX delay disabled
PHY_INTERFACE_MODE_RGMII_TXID: TX delay enabled, RX delay disabled

What I don't like with this patch, is that if we specify phy-mode
PHY_INTERFACE_MODE_RGMII_TXID, this patch will enable TX delay,
but RX delay will not be explicitly set.

Thus it will use the default value of the PHY, and for this PHY
both TX and RX delays are enabled by default.

This means that someone specifying PHY_INTERFACE_MODE_RGMII_TXID
will actually be getting PHY_INTERFACE_MODE_RGMII_ID.


Marc's patch:
https://www.spinics.net/lists/netdev/msg445053.html

does explicitly either enable or disable each delay
(so it does not depend on default values).


However, Marc's patch was never merged, because someone reported
a regression on am335x-evm.


On Vinod's V1 submission Andrew replied that if this change
breaks some existing DT (because that DT specifies a phy-mode
that does not match with reality), then we should fix so that
DT specifies the correct phy-mode:
https://www.spinics.net/lists/netdev/msg542638.html

IMHO, this makes most sense, since if a DT specifies an
incorrect phy-mode, then that is what the user should get.


Kind regards,
Niklas

Powered by blists - more mailing lists