Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
References: <20230920115935.144391-1-biju.das.jz@bp.renesas.com>
In-Reply-To: <20230920115935.144391-1-biju.das.jz@bp.renesas.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Wed, 20 Sep 2023 14:24:23 +0200
Message-ID: <CAMuHMdWp_U_k5nSgmNV6whdK=pNr14h2dS=NetJXRgVEDSSK_g@mail.gmail.com>
Subject: Re: [PATCH] alarmtimer: Fix rebind failure
To:     Biju Das <biju.das.jz@bp.renesas.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Alessandro Zummo <a.zummo@towertech.it>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        John Stultz <jstultz@google.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Biju Das <biju.das.au@gmail.com>, linux-rtc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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

-- 
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