Re: [PATCH] alarmtimer: Fix rebind failure

[Geert Uytterhoeven]

Hi Biju,

On Wed, Sep 20, 2023 at 1:59 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> The resources allocated in alarmtimer_rtc_add_device() are not freed
> leading to re-bind failure for the endpoint driver. Fix this issue
> by adding alarmtimer_rtc_remove_device().
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

Thanks for your patch!

Does this need a Fixes tag?

> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -61,6 +61,7 @@ static DEFINE_SPINLOCK(freezer_delta_lock);
>  /* rtc timer and device for setting alarm wakeups at suspend */
>  static struct rtc_timer                rtctimer;
>  static struct rtc_device       *rtcdev;
> +static struct platform_device  *rtc_pdev;
>  static DEFINE_SPINLOCK(rtcdev_lock);
>
>  /**
> @@ -109,6 +110,7 @@ static int alarmtimer_rtc_add_device(struct device *dev)
>                 }
>
>                 rtcdev = rtc;
> +               rtc_pdev = pdev;
>                 /* hold a reference so it doesn't go away */
>                 get_device(dev);
>                 pdev = NULL;
> @@ -123,6 +125,23 @@ static int alarmtimer_rtc_add_device(struct device *dev)
>         return ret;
>  }
>
> +static void alarmtimer_rtc_remove_device(struct device *dev)
> +{
> +       struct rtc_device *rtc = to_rtc_device(dev);
> +
> +       if (rtc_pdev) {

As the return value of class_interface.add_dev() is never checked
(alarmtimer_rtc_add_device() returns -EBUSY on adding a second
alarmtimer), multiple timers may have been added, but only one of them
will be the real alarmtimer.
Hence this function should check if rtcdev == rtc before unregistering
the real alarmtimer.  Of course all of this should be protected by
rtcdev_lock.

> +               module_put(rtc->owner);
> +               if (device_may_wakeup(rtc->dev.parent))
> +                       device_init_wakeup(&rtc_pdev->dev, false);
> +
> +               platform_device_unregister(rtc_pdev);
> +               put_device(dev);

Perhaps use the reverse order of operations as in
alarmtimer_rtc_add_device()?

> +       }
> +
> +       rtcdev = NULL;
> +       rtc_pdev = NULL;
> +}
> +
>  static inline void alarmtimer_rtc_timer_init(void)
>  {
>         rtc_timer_init(&rtctimer, NULL, NULL);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds