Re: [PATCH net v3] net: fec: Propagate PTP initialization errors

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260904105542.70540-1-phucduc.bui@xxxxxxxxx?part=1
---
net: fec: Propagate PTP initialization errors

Change fec_ptp_init() to return an error code instead of silently
ignoring failures during PTP initialization.

Link: https://lore.kernel.org/all/20260903092430.354186-1-phucduc.bui@xxxxxxxxx/

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..a9a458703547d 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
[ ... ]
> @@ -5394,7 +5397,9 @@ fec_probe(struct platform_device *pdev)
>  	for (i = 0; i < irq_cnt; i++) {
>  		snprintf(irq_name, sizeof(irq_name), "int%d", i);
>  		irq = platform_get_irq_byname_optional(pdev, irq_name);
> -		if (irq < 0)
> +		if (irq < 0 && irq != -ENXIO)
> +			return irq;

[Severity: High]
Does this direct return bypass the error handling cleanup at the end of
fec_probe()?

By returning directly instead of jumping to the failed_irq label, the driver
bypasses fec_ptp_stop(). This leaves the fep->time_keep delayed work
(scheduled earlier in fec_ptp_init()) active.

It also bypasses free_netdev(ndev), which leaks the net_device structure.

When the driver core cleans up devres allocations on probe failure, it unmaps
fep->hwp. If the un-canceled delayed work subsequently fires, will it attempt
to access the unmapped memory at fep->hwp and cause a kernel panic?

[ ... ]
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..557a76797ebaa 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
[ ... ]
> @@ -779,26 +779,32 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  		      HRTIMER_MODE_REL);
>  
>  	irq = platform_get_irq_byname_optional(pdev, "pps");
[ ... ]
> +	if (irq > 0) {
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>  				       0, pdev->name, ndev);
>  		if (ret < 0)
> -			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
> -				 ret);
> +			return ret;
>  	}
>  
>  	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
>  	if (IS_ERR(fep->ptp_clock)) {
> +		ret = PTR_ERR(fep->ptp_clock);
>  		fep->ptp_clock = NULL;
>  		dev_err(&pdev->dev, "ptp_clock_register failed\n");
> +		return ret;

[Severity: High]
Does this error return leave the devm-managed IRQ active while propagating an
error back to the probe function?

If ptp_clock_register() fails, fec_ptp_init() returns an error which
propagates to fec_probe(), causing it to jump to failed_reset.

The error path in fec_probe() manually frees ndev via free_netdev(ndev).

However, the PPS interrupt was registered just prior using devm_request_irq(),
so it remains active until fec_probe() returns and devres tears it down.

If the PPS interrupt fires between the manual free of ndev and the devres
cleanup, will fec_pps_interrupt() cast dev_id to ndev and attempt to
dereference the already-freed ndev structure, causing a use-after-free crash?