[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMuHMdWp_U_k5nSgmNV6whdK=pNr14h2dS=NetJXRgVEDSSK_g@mail.gmail.com>
Date: Wed, 20 Sep 2023 14:24:23 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Biju Das <biju.das.jz@...renesas.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Alessandro Zummo <a.zummo@...ertech.it>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
John Stultz <jstultz@...gle.com>,
Stephen Boyd <sboyd@...nel.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Biju Das <biju.das.au@...il.com>, linux-rtc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH] alarmtimer: Fix rebind failure
Hi Biju,
On Wed, Sep 20, 2023 at 1:59 PM Biju Das <biju.das.jz@...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@...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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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
Powered by blists - more mailing lists