diff mbox series

alarmtimer: Fix rebind failure

Message ID 20230920115935.144391-1-biju.das.jz@bp.renesas.com
State New
Headers
Series alarmtimer: Fix rebind failure |

Commit Message

Biju Das Sept. 20, 2023, 11:59 a.m. UTC 
  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@bp.renesas.com>
---
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(+)

Comments

Geert Uytterhoeven Sept. 20, 2023, 12:24 p.m. UTC | #1 
Hi Biju,

On Wed, Sep 20, 2023 at 1:59 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com>

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
Biju Das Sept. 20, 2023, 12:47 p.m. UTC | #2 
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@bp.renesas.com>
> 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@bp.renesas.com>
> 
> 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
Alexandre Belloni Sept. 20, 2023, 1:26 p.m. UTC | #3 
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 <biju.das.jz@bp.renesas.com>
> ---
> 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
>
Biju Das Sept. 20, 2023, 1:31 p.m. UTC | #4 
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 <biju.das.jz@bp.renesas.com>
> > ---
> > 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
> >
> 
> --
>
Biju Das Sept. 20, 2023, 1:33 p.m. UTC | #5 
> 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 <biju.das.jz@bp.renesas.com>
> > > ---
> > > 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
> > >
> >
> > --
> >
Biju Das Sept. 22, 2023, 7:14 a.m. UTC | #6 
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@bp.renesas.com>
> > 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@bp.renesas.com>
> >
> > 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
diff mbox series

Patch

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)