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 <[email protected]>
---
This issue is found while adding irq support for built in RTC
found on Renesas PMIC RAA215300 device. This issue should present
on all RTC drivers which calls device_init_wakeup() in probe().
---
kernel/time/alarmtimer.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 8d9f13d847f0..592668136bb5 100644
--- 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) {
+ 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);
+ }
+
+ rtcdev = NULL;
+ rtc_pdev = NULL;
+}
+
static inline void alarmtimer_rtc_timer_init(void)
{
rtc_timer_init(&rtctimer, NULL, NULL);
@@ -130,6 +149,7 @@ static inline void alarmtimer_rtc_timer_init(void)
static struct class_interface alarmtimer_rtc_interface = {
.add_dev = &alarmtimer_rtc_add_device,
+ .remove_dev = &alarmtimer_rtc_remove_device,
};
static int alarmtimer_rtc_interface_setup(void)
--
2.25.1
Hi Biju,
On Wed, Sep 20, 2023 at 1:59 PM Biju Das <[email protected]> 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 <[email protected]>
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 -- [email protected]
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
Hi Alexandre Belloni,
> Subject: Re: [PATCH] alarmtimer: Fix rebind failure
>
> On 20/09/2023 12:59:35+0100, Biju Das 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 <[email protected]>
> > ---
> > This issue is found while adding irq support for built in RTC found on
> > Renesas PMIC RAA215300 device. This issue should present on all RTC
> > drivers which calls device_init_wakeup() in probe().
> > ---
> > kernel/time/alarmtimer.c | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index
> > 8d9f13d847f0..592668136bb5 100644
> > --- 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;
>
> This is the alarmtimer pdev, not the RTC one, right?
Yes, it is alarmtimer pdev.
Cheers,
Biju
>
> > 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) {
> > + 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);
> > + }
> > +
> > + rtcdev = NULL;
> > + rtc_pdev = NULL;
> > +}
> > +
> > static inline void alarmtimer_rtc_timer_init(void) {
> > rtc_timer_init(&rtctimer, NULL, NULL); @@ -130,6 +149,7 @@ static
> > inline void alarmtimer_rtc_timer_init(void)
> >
> > static struct class_interface alarmtimer_rtc_interface = {
> > .add_dev = &alarmtimer_rtc_add_device,
> > + .remove_dev = &alarmtimer_rtc_remove_device,
> > };
> >
> > static int alarmtimer_rtc_interface_setup(void)
> > --
> > 2.25.1
> >
>
> --
>
On 20/09/2023 12:59:35+0100, Biju Das 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 <[email protected]>
> ---
> This issue is found while adding irq support for built in RTC
> found on Renesas PMIC RAA215300 device. This issue should present
> on all RTC drivers which calls device_init_wakeup() in probe().
> ---
> kernel/time/alarmtimer.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 8d9f13d847f0..592668136bb5 100644
> --- 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;
This is the alarmtimer pdev, not the RTC one, right?
> 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) {
> + 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);
> + }
> +
> + rtcdev = NULL;
> + rtc_pdev = NULL;
> +}
> +
> static inline void alarmtimer_rtc_timer_init(void)
> {
> rtc_timer_init(&rtctimer, NULL, NULL);
> @@ -130,6 +149,7 @@ static inline void alarmtimer_rtc_timer_init(void)
>
> static struct class_interface alarmtimer_rtc_interface = {
> .add_dev = &alarmtimer_rtc_add_device,
> + .remove_dev = &alarmtimer_rtc_remove_device,
> };
>
> static int alarmtimer_rtc_interface_setup(void)
> --
> 2.25.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
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 <[email protected]>
> 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 <[email protected]>
>
> 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
> Subject: RE: [PATCH] alarmtimer: Fix rebind failure
>
> Hi Alexandre Belloni,
>
> > Subject: Re: [PATCH] alarmtimer: Fix rebind failure
> >
> > On 20/09/2023 12:59:35+0100, Biju Das 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 <[email protected]>
> > > ---
> > > This issue is found while adding irq support for built in RTC found
> > > on Renesas PMIC RAA215300 device. This issue should present on all
> > > RTC drivers which calls device_init_wakeup() in probe().
> > > ---
> > > kernel/time/alarmtimer.c | 20 ++++++++++++++++++++
> > > 1 file changed, 20 insertions(+)
> > >
> > > diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> > > index
> > > 8d9f13d847f0..592668136bb5 100644
> > > --- 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;
> >
> > This is the alarmtimer pdev, not the RTC one, right?
>
> Yes, it is alarmtimer pdev.
OK, I will change it to alarmtimer_pdev to avoid confusion.
Cheers,
Biju
>
> >
> > > 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) {
> > > + 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);
> > > + }
> > > +
> > > + rtcdev = NULL;
> > > + rtc_pdev = NULL;
> > > +}
> > > +
> > > static inline void alarmtimer_rtc_timer_init(void) {
> > > rtc_timer_init(&rtctimer, NULL, NULL); @@ -130,6 +149,7 @@ static
> > > inline void alarmtimer_rtc_timer_init(void)
> > >
> > > static struct class_interface alarmtimer_rtc_interface = {
> > > .add_dev = &alarmtimer_rtc_add_device,
> > > + .remove_dev = &alarmtimer_rtc_remove_device,
> > > };
> > >
> > > static int alarmtimer_rtc_interface_setup(void)
> > > --
> > > 2.25.1
> > >
> >
> > --
> >
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 <[email protected]>
> > 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 <[email protected]>
> >
> > 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