RE: [PATCH] alarmtimer: Fix rebind failure

[Biju Das]

Hi Geert Uytterhoeven,

> Subject: RE: [PATCH] alarmtimer: Fix rebind failure
> 
> 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.

I won't be able to add lock here as it is giving

1) BUG invalid context
2) Scheduling while atomic() as lock is held by delete function.

Cheers,
Biju