netdev - Re: [PATCH net-next] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

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: <fbkzmsznag5yjypbzmbmvtzfgdgx3v4pc6njmelrz3x7pvlojq@rh3tqyo5sr26>
Date: Tue, 26 Sep 2023 14:39:21 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, Jose.Abreu@...opsys.com, 
	linux@...linux.org.uk, hkallweit1@...il.com, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next] net: pcs: xpcs: Add 2500BASE-X case in get
 state for XPCS drivers

Hi Raju

On Mon, Sep 25, 2023 at 01:21:42PM +0530, Raju Lakkaraju wrote:
> Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause
> Update the port mode and autonegotiation
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
> ---
>  drivers/net/pcs/pcs-xpcs.c | 31 +++++++++++++++++++++++++++++++
>  drivers/net/pcs/pcs-xpcs.h |  4 ++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 4dbc21f604f2..4f89dcedf0fc 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -1090,6 +1090,30 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
>  	return 0;
>  }
>  
> +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs,
> +				    struct phylink_link_state *state)
> +{
> +	int sts, lpa;
> +
> +	sts = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS);

> +	lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_LP_BABL);
> +	if (sts < 0 || lpa < 0) {
> +		state->link = false;
> +		return sts;
> +	}

The HW manual says: "The host uses this page to know the link
partner's ability when the base page is received through Clause 37
auto-negotiation." Seeing xpcs_config_2500basex() disables
auto-negotiation and lpa value is unused anyway why do you even need
to read the LP_BABL register?

> +
> +	state->link = !!(sts & DW_VR_MII_MMD_STS_LINK_STS);

> +	state->an_complete = !!(sts & DW_VR_MII_MMD_STS_AN_CMPL);

Similarly AN is disabled in the xpcs_config_2500basex() method. Why do
you need the parsing above? It isn't supposed to indicate any useful
value since AN is disabled.

-Serge(y)

> +	if (!state->link)
> +		return 0;
> +
> +	state->speed = SPEED_2500;
> +	state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX;
> +	state->duplex = DUPLEX_FULL;
> +
> +	return 0;
> +}
> +
>  static void xpcs_get_state(struct phylink_pcs *pcs,
>  			   struct phylink_link_state *state)
>  {
> @@ -1127,6 +1151,13 @@ static void xpcs_get_state(struct phylink_pcs *pcs,
>  			       ERR_PTR(ret));
>  		}
>  		break;
> +	case DW_2500BASEX:
> +		ret = xpcs_get_state_2500basex(xpcs, state);
> +		if (ret) {
> +			pr_err("xpcs_get_state_2500basex returned %pe\n",
> +			       ERR_PTR(ret));
> +		}
> +		break;
>  	default:
>  		return;
>  	}
> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index 39a90417e535..92c838f4b251 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -55,6 +55,10 @@
>  /* Clause 37 Defines */
>  /* VR MII MMD registers offsets */
>  #define DW_VR_MII_MMD_CTRL		0x0000
> +#define DW_VR_MII_MMD_STS		0x0001
> +#define DW_VR_MII_MMD_STS_LINK_STS	BIT(2)
> +#define DW_VR_MII_MMD_STS_AN_CMPL	BIT(5)
> +#define DW_VR_MII_MMD_LP_BABL		0x0005
>  #define DW_VR_MII_DIG_CTRL1		0x8000
>  #define DW_VR_MII_AN_CTRL		0x8001
>  #define DW_VR_MII_AN_INTR_STS		0x8002
> -- 
> 2.34.1
> 
>