Re: [net-next 2/9] net: ethernet: ravb: Move programming of gPTP timer interval

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The gPTP initialization sequence incorrectly waits on the GCCR_TCR field instead of GCCR_LTI after requesting a timer increment load.

Pre-existing issues:
- [High] ravb_ptp_init() assigns the result of ptp_clock_register() to priv->ptp.clock without an IS_ERR() check, leading to invalid pointer dereferences.
--

commit 105bd561296a957da9d4e09fecae37137d532c98
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Move programming of gPTP timer interval

This commit moves the programming of the gPTP timer interval into the gPTP
initialization function. This allows the programming to occur while holding
the gPTP registers lock.

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 226c6c0ab945b..7c78f75cb284a 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
[ ... ]
> @@ -319,11 +319,20 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
[ ... ]
>  	spin_lock_irqsave(&priv->lock, flags);
> +
> +	/* Set gPTP Timer Increment Value. */
> +	ravb_write(ndev, priv->ptp.default_addend, GTI);
> +
> +	/* Request GTI loading. */
> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +
> +	/* Wait for GIT loading to complete. */
>  	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);

[Severity: Medium]
The commit message states the goal is to wait for the timer interval
programming to be acknowledged by the hardware. Does waiting on GCCR_TCR
achieve this?

The code requests the load by setting GCCR_LTI, but the wait immediately
follows on GCCR_TCR. Since GCCR_TCR is typically 0 at this stage, the wait
will return immediately instead of waiting for the hardware to clear GCCR_LTI.

Could the subsequent ravb_modify() on GCCR execute before the hardware
clears GCCR_LTI, potentially writing a 1 back to it and disrupting the
initialization sequence?

> +
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
>  	spin_unlock_irqrestore(&priv->lock, flags);

[Severity: High]
This is a pre-existing issue, but looking at the rest of ravb_ptp_init()
just below this lock:

	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
}

Should there be an IS_ERR() check here?

If ptp_clock_register() fails, it returns an ERR_PTR. Because the driver
does not validate the return value, priv->ptp.clock would hold this error
pointer.

When the interface is brought down via ravb_ptp_stop(), or when a hardware
PTP interrupt occurs, this pointer is passed to ptp_clock_unregister() or
ptp_clock_event(). Since neither function checks for error pointers, would
this lead to a panic from an invalid pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610102432.3538432-3-niklas.soderlund+renesas@ragnatech.se?part=1