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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, May 29, 2026 at 06:21:17PM +0000, sashiko-bot@kernel.org wrote:
> 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.

No it cannot. default_pccr[mode] == 0 is not a valid mux configuration,
it universally means the protocol converter is disabled.

> > +		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.

This is actually a great comment. Re-reading the hardware documentation,
the power up sequence is indeed messed up. Will fix for v3.

> > +
> > +	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?

Will fix in v3.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy