Re: [PATCH v2 net-next 06/14] net: enetc: simplify enetc4_set_port_speed()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removal of the switch statement allows SPEED_UNKNOWN to be programmed as an invalid hardware state.
--

commit 923303ff32884703a5eb3f948098684f15149ba1
Author: Wei Fang <wei.fang@nxp.com>

net: enetc: simplify enetc4_set_port_speed()

This commit simplifies enetc4_set_port_speed() by removing a switch
statement, assuming phylink only passes valid speed values. It also
updates PCR_PSPEED_VAL() to use FIELD_PREP() instead of an open-coded
shift.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
> index 6a8f2ed56017c..dea1fd0b81758 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
> @@ -148,7 +148,7 @@
>  #define  PCR_L2DOSE			BIT(4)
>  #define  PCR_TIMER_CS			BIT(8)
>  #define  PCR_PSPEED			GENMASK(29, 16)
> -#define  PCR_PSPEED_VAL(speed)		(((speed) / 10 - 1) << 16)
> +#define  PCR_PSPEED_VAL(s)		FIELD_PREP(PCR_PSPEED, ((s) / 10 - 1))

[Severity: High]
Does this safely handle negative values like SPEED_UNKNOWN?

If SPEED_UNKNOWN (-1) is passed here, the expression ((s) / 10 - 1)
evaluates to -1. FIELD_PREP then casts this to an unsigned type and masks
it against PCR_PSPEED, filling all 14 bits with 1s (0x3FFF).

>  
>  /* Port MAC address register 0/1 */
>  #define ENETC4_PMAR0			0x4020
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index b966637572a7a..db7bad2c3cbde 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
[ ... ]
> -	switch (speed) {
> -	case SPEED_100:
> -	case SPEED_1000:
> -	case SPEED_2500:
> -	case SPEED_10000:
> -		val |= (PCR_PSPEED & PCR_PSPEED_VAL(speed));
> -		break;
> -	case SPEED_10:
> -	default:
> -		val |= (PCR_PSPEED & PCR_PSPEED_VAL(SPEED_10));
> -	}
> -
> -	priv->speed = speed;
> +	val = enetc_port_rd(&priv->si->hw, ENETC4_PCR) & (~PCR_PSPEED);
> +	val |= PCR_PSPEED_VAL(speed);

[Severity: High]
The commit message states: "Since phylink only passes valid speed values
to mac_link_up, the switch statement with its default fallback to SPEED_10
is unnecessary."

Is this assumption accurate?

In the kernel phylib/phylink implementation, if a PHY establishes link before
autonegotiation completes (or if an out-of-band PCS status hasn't fully
resolved the speed), genphy_read_status() can leave phydev->speed as
SPEED_UNKNOWN (-1) while phydev->link is 1.

Phylink then propagates this state via phylink_resolve() and calls
mac_link_up() with SPEED_UNKNOWN.

Without the switch statement's default case, won't this write the 0x3FFF
garbage value generated by PCR_PSPEED_VAL() to the MAC speed configuration
register (ENETC4_PCR) and cause undefined hardware behavior?

>  	enetc_port_wr(&priv->si->hw, ENETC4_PCR, val);
> +	priv->speed = speed;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702025714.456233-1-wei.fang@oss.nxp.com?part=6