| Message ID | 20230920115935.144391-1-biju.das.jz@bp.renesas.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | alarmtimer: Fix rebind failure | expand |
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
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
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 >
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 > > > > -- >
> 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 > > > > > > > -- > >
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 --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)
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(+)