netdev - Re: [PATCH net-next v3 4/7] net: phy: aquantia: add essential functions to aqr105 driver

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: <9a0491f6-9743-4588-a3a0-30110e2c7261@lunn.ch>
Date: Thu, 19 Dec 2024 17:39:24 +0100
From: Andrew Lunn <andrew@...n.ch>
To: hfdevel@....net
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	FUJITA Tomonori <fujita.tomonori@...il.com>,
	Andrew Lunn <andrew+netdev@...n.ch>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 4/7] net: phy: aquantia: add essential
 functions to aqr105 driver

On Tue, Dec 17, 2024 at 10:07:35PM +0100, Hans-Frieder Vogt via B4 Relay wrote:
> From: Hans-Frieder Vogt <hfdevel@....net>
> 
> This patch makes functions that were provided for aqr107 applicable to
> aqr105, or replaces generic functions with specific ones. Since the aqr105
> was introduced before NBASE-T was defined (or 802.3bz), there are a number
> of vendor specific registers involved in the definition of the
> advertisement, in auto-negotiation and in the setting of the speed. The
> functions have been written following the downstream driver for TN4010
> cards with AQR105 PHY. To avoid duplication of code, aqr_config_aneg was
> split in a common part and a specific part for aqr105 and other aqr PHYs.
> A similar approach has been chosen for the aqr107_read_status function.
> Here, the aqr generation (=1 for aqr105, and =2 for aqr107 and higher) is
> used to decide whether aqr107(and up) specific registers can be used. I
> know this is not particularly elegant, but it is doing the job.
> 
> Signed-off-by: Hans-Frieder Vogt <hfdevel@....net>
> ---
>  drivers/net/phy/aquantia/aquantia_main.c | 168 ++++++++++++++++++++++++++-----
>  1 file changed, 144 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index 81eeee29ba3e6fb11a476a5b51a8a8be061ca8c3..a112e3473e079822671535c313f3ae816fe186dd 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -33,6 +33,9 @@
>  #define PHY_ID_AQR115C	0x31c31c33
>  #define PHY_ID_AQR813	0x31c31cb2
>  
> +#define MDIO_AN_10GBT_CTRL_ADV_LTIM		BIT(0)
> +#define ADVERTISE_XNP				BIT(12)
> +
>  #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
> @@ -50,6 +53,7 @@
>  #define MDIO_AN_VEND_PROV_1000BASET_HALF	BIT(14)
>  #define MDIO_AN_VEND_PROV_5000BASET_FULL	BIT(11)
>  #define MDIO_AN_VEND_PROV_2500BASET_FULL	BIT(10)
> +#define MDIO_AN_VEND_PROV_EXC_PHYID_INFO	BIT(6)
>  #define MDIO_AN_VEND_PROV_DOWNSHIFT_EN		BIT(4)
>  #define MDIO_AN_VEND_PROV_DOWNSHIFT_MASK	GENMASK(3, 0)
>  #define MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT	4
> @@ -107,6 +111,30 @@
>  #define AQR107_OP_IN_PROG_SLEEP		1000
>  #define AQR107_OP_IN_PROG_TIMEOUT	100000
>  
> +static int aqr105_get_features(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Normal feature discovery */
> +	ret = genphy_c45_pma_read_abilities(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* The AQR105 PHY misses to indicate the 2.5G and 5G modes, so add them
> +	 * here
> +	 */
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			 phydev->supported);
> +
> +	/* The AQR105 PHY suppports both RJ45 and SFP+ interfaces */
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_TP_BIT, phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
> +
> +	return 0;
> +}
> +
>  static int aqr107_get_sset_count(struct phy_device *phydev)
>  {
>  	return AQR107_SGMII_STAT_SZ;
> @@ -164,6 +192,59 @@ static void aqr107_get_stats(struct phy_device *phydev,
>  	}
>  }
>  
> +static int aqr105_config_speed(struct phy_device *phydev)
> +{
> +	int vend = MDIO_AN_VEND_PROV_EXC_PHYID_INFO;
> +	int ctrl10 = MDIO_AN_10GBT_CTRL_ADV_LTIM;
> +	int adv = ADVERTISE_CSMA;
> +	int ret;
> +
> +	/* Half duplex is not supported */
> +	if (phydev->duplex != DUPLEX_FULL)
> +		return -EINVAL;

Do the half duplex modes make into the phydev->supported modes?  I
would expect the core to raise an error if asked to do half duplex but
it is not in phydev->supported.

> +static int aqr_common_read_rate(struct phy_device *phydev, int aqr_gen)
>  {
>  	u32 config_reg;
>  	int val;
> @@ -377,20 +482,22 @@ static int aqr107_read_rate(struct phy_device *phydev)
>  		return 0;
>  	}
>  
> -	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, config_reg);
> -	if (val < 0)
> -		return val;
> +	if (aqr_gen > 1) {
> +		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, config_reg);
> +		if (val < 0)
> +			return val;
>  
> -	if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) ==
> -	    VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
> -		phydev->rate_matching = RATE_MATCH_PAUSE;
> -	else
> -		phydev->rate_matching = RATE_MATCH_NONE;
> +		if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) ==
> +		    VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
> +			phydev->rate_matching = RATE_MATCH_PAUSE;
> +		else
> +			phydev->rate_matching = RATE_MATCH_NONE;
> +	}

This appears to be at the end of read_rate(), which is also called at
the end of read_status. 

> +static int aqr105_read_status(struct phy_device *phydev)
> +{
> +	return aqr_common_read_status(phydev, 1);
> +}
> +
> +static int aqr107_read_status(struct phy_device *phydev)
> +{
> +	return aqr_common_read_status(phydev, 2);

So rather than having this 1, 2, i think you can put the code here.

	Andrew