Re: [PATCH net-next v4 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing WRITE_ONCE and temporary exposure of ERR_PTR in a locklessly accessed variable.
--

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

net: ethernet: ravb: Propagate error from ptp_clock_register()

This commit reworks ravb_ptp_init() to check the return value of
ptp_clock_register() and propagate any errors back to the caller. It also
refactors the clock assignment by removing the intermediate local variable.

Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index f70b616ac847c..37161c0579ce7 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
[ ... ]
> -void ravb_ptp_init(struct net_device *ndev)
> +int ravb_ptp_init(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ptp_clock *clock;
>  	unsigned long flags;
> +	int ret = 0;
>  
>  	priv->ptp.info = ravb_ptp_info;
>  
[ ... ]
> -	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
> -	if (IS_ERR(clock)) {
> -		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
> -		clock = NULL;
> +	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
> +	if (IS_ERR(priv->ptp.clock)) {
> +		ret = PTR_ERR(priv->ptp.clock);
> +		priv->ptp.clock = NULL;
> +		ravb_ptp_stop(ndev);
>  	}
>  
> -	WRITE_ONCE(priv->ptp.clock, clock);
> -	if (clock)
> -		WRITE_ONCE(priv->ptp.phc_index, ptp_clock_index(clock));
> +	return ret;
>  }

[Severity: Medium]
Does this introduce a data race by directly publishing an invalid pointer?

The ravb_ptp_interrupt() handler accesses priv->ptp.clock locklessly via
READ_ONCE(). Because the interrupt handler is registered and enabled via
ravb_setup_irq() before ravb_ptp_init() runs, the hardware interrupt might
already be firing when we reach this point.

If ptp_clock_register() fails, it returns an ERR_PTR. By assigning it
directly to priv->ptp.clock without WRITE_ONCE(), we could have a scenario
where an interrupt fires immediately after the assignment, but before the
error check clears priv->ptp.clock to NULL.

The interrupt handler in ravb_ptp_interrupt() would then read the ERR_PTR:

	struct ptp_clock *clock = READ_ONCE(priv->ptp.clock);
	...
	if ((gis & GIS_PTCF) && clock) {
		...
		ptp_clock_event(clock, &event);

Could this crash by dereferencing the ERR_PTR? Would it be safer to
restore the original local variable pattern and retain the WRITE_ONCE()
so that priv->ptp.clock only ever sees a valid pointer or NULL?

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