Hi Geert Uytterhoeven,
Thanks for the feedback.
> Subject: Re: [PATCH] alarmtimer: Fix rebind failure
>
> 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?
I think so, as it breaks unbind/bind on lot of RTC drivers.
There are 2 commits, I will add both as fixes tag.
c79108bd19a8 ("alarmtimer: Make alarmtimer platform device child of RTC device")
7c94caca877b ("alarmtimer: Use wakeup source from alarmtimer platform device"
>
> > --- 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.
Ok will add lock here and the check.
>
> > + 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()?
Platform device is child of rtc device. So it has to be
at the last as already there is put_device() call in
devm_rtc_release_device()
Cheers,
Biju
>
> > + }
> > +
> > + 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@linux-
> m68k.org
>
> 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
