netdev - Re: [PATCH net-next 1/1] net: phy: marvell10g: Add PHY loopback support for 88E2110 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] [day] [month] [year] [list]

Date:   Tue, 23 Mar 2021 10:37:53 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Wong Vee Khee <vee.khee.wong@...ux.intel.com>,
        Russell King <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Voon Weifeng <weifeng.voon@...el.com>,
        Ong Boong Leong <boon.leong.ong@...el.com>
Subject: Re: [PATCH net-next 1/1] net: phy: marvell10g: Add PHY loopback
 support for 88E2110 PHY

On 23.03.2021 09:48, Wong Vee Khee wrote:
> From: Tan Tee Min <tee.min.tan@...el.com>
> 
> Add support for PHY loopback for the Marvell 88E2110 PHY.
> 
> This allow user to perform selftest using ethtool.
> 
> Signed-off-by: Tan Tee Min <tee.min.tan@...el.com>
> Signed-off-by: Wong Vee Khee <vee.khee.wong@...ux.intel.com>
> ---
>  drivers/net/phy/marvell10g.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index b1bb9b8e1e4e..c45a8f11bdcf 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -89,6 +89,8 @@ enum {
>  	MV_V2_TEMP_CTRL_DISABLE	= 0xc000,
>  	MV_V2_TEMP		= 0xf08c,
>  	MV_V2_TEMP_UNKNOWN	= 0x9600, /* unknown function */
> +
> +	MV_LOOPBACK		= BIT(14), /* Loopback (88E2110 only) */

Why do you state 88E2110 only?
This is the standard PCS loopback bit as described in clause 45.2.3.1.2
It's defined already as MDIO_PCS_CTRL1_LOOPBACK.
E.g. the 88x3310 spec also describes this bit.

>  };
>  
>  struct mv3310_priv {
> @@ -765,6 +767,15 @@ static int mv3310_set_tunable(struct phy_device *phydev,
>  	}
>  }
>  
> +static int mv3310_loopback(struct phy_device *phydev, bool enable)
> +{
> +	if (phydev->drv->phy_id != MARVELL_PHY_ID_88E2110)
> +		return -EOPNOTSUPP;

If you use the function in the 2110 PHY driver only, then why this check?
And why name it 3310 if it can be used with 2110 only?

This function uses c45 standard functionality only, therefore it should
go to generic code (similar to genphy_loopback for c22).


> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T,
> +			      MV_LOOPBACK, enable ? MV_LOOPBACK : 0);
> +}
> +
>  static struct phy_driver mv3310_drivers[] = {
>  	{
>  		.phy_id		= MARVELL_PHY_ID_88X3310,
> @@ -796,6 +807,7 @@ static struct phy_driver mv3310_drivers[] = {
>  		.get_tunable	= mv3310_get_tunable,
>  		.set_tunable	= mv3310_set_tunable,
>  		.remove		= mv3310_remove,
> +		.set_loopback	= mv3310_loopback,
>  	},
>  };
>  
>