RE: [PATCH net v2 2/2] net: fec: free the PPS interrupt before tearing down the PHC and netdev

[Wei Fang]

> The devm-managed PPS interrupt remains registered until after the
> remove callback returns, outliving both the PHC and the netdev.
> An in-flight handler can call ptp_clock_event() during PHC teardown,
> and a later interrupt can dereference the freed netdev.
>
> Record the IRQ after a successful request and release it with
> devm_free_irq() in fec_ptp_stop(), before ptp_clock_unregister().
> This removes the handler and waits for any running instance to
> finish before the PHC and netdev are torn down.
>
> Fixes: 4ad1ceec05e4 ("net: fec: Let fec_ptp have its own interrupt routine")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: GLM:5.3
> Signed-off-by: Shengzhuo Wei <me@xxxxxxxx>
> ---
>  drivers/net/ethernet/freescale/fec.h     |  1 +
>  drivers/net/ethernet/freescale/fec_ptp.c | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index 7176803146f3..960b9f01c531 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -670,6 +670,7 @@ struct fec_enet_private {
>
>       /* pps  */
>       int pps_channel;
> +     int pps_irq;
>       unsigned int reload_period;
>       int pps_enable;
>       unsigned int next_counter;
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d5..567ea2a46117 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -778,6 +778,7 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>       hrtimer_setup(&fep->perout_timer, fec_ptp_pps_perout_handler,
> CLOCK_REALTIME,
>                     HRTIMER_MODE_REL);
>
> +     fep->pps_irq = -1;
>       irq = platform_get_irq_byname_optional(pdev, "pps");
>       if (irq < 0)
>               irq = platform_get_irq_optional(pdev, irq_idx);
> @@ -787,9 +788,12 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>       if (irq >= 0) {
>               ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>                                      0, pdev->name, ndev);
> -             if (ret < 0)
> +             if (ret < 0) {
>                       dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
>                                ret);
> +             } else {
> +                     fep->pps_irq = irq;
> +             }

braces {} are not necessary for single statement blocks

>       }
>
>       fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
> @@ -861,6 +865,12 @@ void fec_ptp_stop(struct platform_device *pdev)
>
>       cancel_delayed_work_sync(&fep->time_keep);
>       hrtimer_cancel(&fep->perout_timer);
> +
> +     if (fep->pps_irq >= 0) {
> +             devm_free_irq(&pdev->dev, fep->pps_irq, ndev);

Since devm_*_irq APIs are useless for PPS irq, I think it is better to use
request_irq() and free_irq() instead.

> +             fep->pps_irq = -1;
> +     }
> +
>       if (fep->ptp_clock)
>               ptp_clock_unregister(fep->ptp_clock);
>  }
>
> --
> 2.47.3

NXP Confidential