Re: [PATCH v2 phy-next 14/15] phy: lynx-10g: new driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Incorrect restoration of the PCCR mux configuration because `0` is treated as uninitialized.
- [High] Potential timing violation when bringing lanes out of reset and power down simultaneously.

Pre-existing issues:
- [High] lynx_xlate can return a NULL pointer for unprobed PHY lanes, leading to a NULL pointer dereference in the PHY core framework.
--

commit 851e6824db7b03958722b9808f3549609168de99
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

phy: lynx-10g: new driver
    
Introduce a driver for the networking lanes of the 10G Lynx SerDes
block, present on the majority of Layerscape and QorIQ (Freescale/NXP)
SoCs.

> diff --git a/drivers/phy/freescale/phy-fsl-lynx-10g.c b/drivers/phy/freescale/phy-fsl-lynx-10g.c
> index 0000000000000..9b04d6a4b825a
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-fsl-lynx-10g.c
[ ... ]
> +static int lynx_10g_lane_enable_pcvt(struct lynx_lane *lane,
> +				     enum lynx_lane_mode mode)
> +{
> +	struct lynx_priv *priv = lane->priv;
> +	u32 val;
> +	int err;
> +
> +	spin_lock(&priv->pcc_lock);
[ ... ]
> +	if (lane->default_pccr[mode]) {
[Severity: High]
Can this condition incorrectly fall through to hardcoded defaults if a valid
mux configuration of 0 is backed up?

Since default_pccr is zero-initialized by devm_kcalloc(), a backed-up value
of 0 evaluates to false. This skips the restoration and allows the driver to
apply hardcoded defaults later, which could permanently reroute the hardware
lanes and break the network interface.

> +		err = lynx_pccr_write(lane, mode, lane->default_pccr[mode]);
> +		goto out;
> +	}
> +
> +	val = 0;
[ ... ]
> +static int lynx_10g_power_on(struct phy *phy)
> +{
> +	struct lynx_lane *lane = phy_get_drvdata(phy);
> +
> +	if (lane->powered_up)
> +		return 0;
> +
> +	/* The RM says to wait for at least 120ns between per lane setting have
> +	 * been changed and the lane being taken out of reset
> +	 */
> +	usleep_range(1, 2);
> +
> +	lynx_lane_rmw(lane, LNaGCR0, LNaGCR0_RRST_OFF | LNaGCR0_TRST_OFF,
> +		      LNaGCR0_RRST | LNaGCR0_TRST |
> +		      LNaGCR0_RX_PD | LNaGCR0_TX_PD);
[Severity: High]
Does this code violate typical PHY hardware sequencing by clearing both the
power-down bits and digital reset bits simultaneously?

Releasing the digital reset at the same time as activating the analog
components doesn't leave time for the analog circuitry to stabilize. The
prior usleep_range() occurs before power-up is initiated, which might not
prevent hardware lockups or unreliable behavior on link up.

> +
> +	lane->powered_up = true;
> +
> +	return 0;
> +}
[ ... ]
> +static int lynx_10g_probe(struct platform_device *pdev)
> +{
> +	return lynx_probe(pdev, of_device_get_match_data(&pdev->dev),
> +			  &lynx_10g_ops);
> +}
[Severity: High]
This is a pre-existing issue, but does the core PHY framework crash if
lynx_xlate() returns a NULL pointer for an unprobed PHY lane?

If a PHY lane is requested via device tree but the driver did not probe it,
priv->lane[idx].phy will be NULL. If lynx_xlate() returns this NULL pointer,
the core PHY framework interprets it as a successful retrieval of an optional
PHY and might attempt to dereference it. Would it be safer to explicitly
return ERR_PTR(-ENODEV) in lynx_xlate() when the phy pointer is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529171509.1163787-1-vladimir.oltean@nxp.com?part=14