|SUCCESS| [v3] net/axgbe: mask unsupported PHY half-duplex modes

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162827

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-02

# Review of net/axgbe: mask unsupported PHY half-duplex modes

git sha: 20260402065450.1074-1-ashokkumar.natarajan at amd.com
Author: Ashok Kumar Natarajan <ashokkumar.natarajan at amd.com>
Subject: [PATCH v3] net/axgbe: mask unsupported PHY half-duplex modes

The patch updates PHY advertisement configuration to mask half-duplex
modes unsupported by the AXGBE MAC. It modifies both Clause 22 (10/100)
and Clause 40 (1000BASE-T) advertisement handling using read-modify-write
operations.

> diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
> index e5568cce5f..3c00adfebc 100644
> --- a/drivers/net/axgbe/axgbe_phy.h
> +++ b/drivers/net/axgbe/axgbe_phy.h
> @@ -156,6 +156,9 @@
>  #define AXGBE_M88E1512_MODE_RGMII_SGMII		4
>  #define AXGBE_M88E1512_MODE_SW_RESET		0x8000
>  
> +/* IEEE 802.3 Clause 40: 1000BASE-T Advertisement Control */
> +#define AXGBE_PHY_MII_CTRL1000_1000T_FULL	0x0200
> +#define AXGBE_PHY_MII_CTRL1000_1000T_HALF	0x0100

These defines appear redundant. The standard Linux kernel headers define
ADVERTISE_1000FULL (0x0200) and ADVERTISE_1000HALF (0x0100) for 1000BASE-T
in linux/mii.h. Does this code need driver-specific duplicates, or could
it use the standard kernel definitions?

>  /* Control register 1. */
>  /* Enable extended speed selection */
> diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
> index 369d766884..0f0c80f127 100644
> --- a/drivers/net/axgbe/axgbe_phy_impl.c
> +++ b/drivers/net/axgbe/axgbe_phy_impl.c

[ ... ]

> @@ -303,23 +303,113 @@ static int axgbe_phy_write(struct axgbe_port *pdata, u16 reg, u16 value)
>  
>  static int axgbe_phy_config_advert(struct axgbe_port *pdata)
>  {
> -	u16 advert;
> +	u32 adv = pdata->phy.advertising;
> +	u16 advert, orig_advert;
> +	u16 ctrl1000, orig_ctrl1000;
>  	int ret;
>  
> +	/*
> +	 * Clause 22 (10/100) advertisement configuration.
> +	 *
> +	 * AXGBE MAC supports only full-duplex operation.
> +	 * Half-duplex modes are masked while preserving any
> +	 * PHY-specific or reserved bits.
> +	 */
>  	ret = pdata->phy_if.phy_impl.read(pdata, MII_ADVERTISE, &advert);
>  	if (ret) {
> -		PMD_DRV_LOG_LINE(ERR, "Failed to read ADVERTISE register");
> +		PMD_DRV_LOG_LINE(ERR,
> +				"PHY read failed: MII_ADVERTISE");

The indentation appears incorrect. The DPDK coding style requires function
arguments to be aligned with the opening parenthesis. This should be:

    PMD_DRV_LOG_LINE(ERR,
                     "PHY read failed: MII_ADVERTISE");

This pattern repeats throughout the patch.

>  		return ret;
>  	}
>  
> -	advert |= ADVERTISE_FULL;
> -	advert |= ADVERTISE_PAUSE_CAP;
> +	orig_advert = advert;
> +
> +	/* Always advertise IEEE 802.3 CSMA/CD selector */
> +	advert |= ADVERTISE_CSMA;
> +
> +	/* AXGBE does not support 10/100 half-duplex */
> +	advert &= ~(ADVERTISE_10HALF | ADVERTISE_100HALF);
> +
> +	/*
> +	 * Treat adv == 0 as a legacy/default configuration where the driver
> +	 * does not impose an explicit policy and preserves the historical
> +	 * behavior of advertising all supported full-duplex speeds.
> +	 */

Does the implicit comparison of adv == 0 violate the coding style
guideline that forbids implicit test for 0/-1? The DPDK coding style
requires explicit comparison:

    if (adv != 0)

rather than:

    if (adv)

> +	if (!adv) {
> +		advert |= ADVERTISE_10FULL | ADVERTISE_100FULL;
> +	} else {
> +		if (adv & ADVERTISED_10baseT_Full)
> +			advert |= ADVERTISE_10FULL;
> +		else
> +			advert &= ~ADVERTISE_10FULL;
> +
> +		if (adv & ADVERTISED_100baseT_Full)
> +			advert |= ADVERTISE_100FULL;
> +		else
> +			advert &= ~ADVERTISE_100FULL;
> +
> +		if (adv & ADVERTISED_Pause)
> +			advert |= ADVERTISE_PAUSE_CAP;
> +		else
> +			advert &= ~ADVERTISE_PAUSE_CAP;
>  
> -	ret = pdata->phy_if.phy_impl.write(pdata, MII_ADVERTISE, advert);
> +		if (adv & ADVERTISED_Asym_Pause)
> +			advert |= ADVERTISE_PAUSE_ASYM;
> +		else
> +			advert &= ~ADVERTISE_PAUSE_ASYM;

The code previously unconditionally advertised ADVERTISE_PAUSE_CAP. Now it
only sets this bit when ADVERTISED_Pause is set in adv, or when adv is 0.
Does this behavioral change potentially break existing configurations that
relied on pause frames being advertised by default?

> +	}
> +
> +	if (advert != orig_advert) {

Same question regarding implicit comparison. Should this be:

    if ((advert ^ orig_advert) != 0)

> +		ret = pdata->phy_if.phy_impl.write(pdata,
> +				MII_ADVERTISE,
> +				advert);

Indentation issue again. Arguments should align with the opening
parenthesis.

> +		if (ret) {
> +			PMD_DRV_LOG_LINE(ERR,
> +					"PHY write failed: MII_ADVERTISE");
> +			return ret;
> +		}
> +	}
> +
> +	/*
> +	 * Clause 40 (1000BASE-T) advertisement configuration.
> +	 *
> +	 * AXGBE MAC supports only full-duplex operation at 1Gbps.
> +	 * Half-duplex advertisement is always cleared.
> +	 * Existing PHY or vendor-specific bits are preserved.
> +	 */
> +	ret = pdata->phy_if.phy_impl.read(pdata, MII_CTRL1000, &ctrl1000);
>  	if (ret) {
> -		PMD_DRV_LOG_LINE(ERR, "Failed to write ADVERTISE register");
> +		PMD_DRV_LOG_LINE(ERR,
> +				"PHY read failed: MII_CTRL1000");
>  		return ret;
>  	}
> +
> +	orig_ctrl1000 = ctrl1000;
> +
> +	/* Clear unsupported 1000BASE-T half-duplex */
> +	ctrl1000 &= ~AXGBE_PHY_MII_CTRL1000_1000T_HALF;
> +
> +	/*
> +	 * As with Clause 22 advertisement, adv == 0 indicates that no explicit
> +	 * advertising policy was requested. In this case, preserve the legacy
> +	 * behaviour of advertising 1000BASE-T full-duplex by default.
> +	 */
> +	if (!adv || (adv & ADVERTISED_1000baseT_Full))
> +		ctrl1000 |= AXGBE_PHY_MII_CTRL1000_1000T_FULL;
> +	else
> +		ctrl1000 &= ~AXGBE_PHY_MII_CTRL1000_1000T_FULL;

Does this code need explicit checks for adv != 0? The coding style
guideline discourages implicit testing.

> +
> +	if (ctrl1000 != orig_ctrl1000) {
> +		ret = pdata->phy_if.phy_impl.write(pdata,
> +				MII_CTRL1000,
> +				ctrl1000);

Indentation issue with function arguments.

> +		if (ret) {
> +			PMD_DRV_LOG_LINE(ERR,
> +					"PHY write failed: MII_CTRL1000");
> +			return ret;
> +		}
> +	}
> +
>  	return 0;
>  }