Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <5629262.DvuYhMxLoT@kreacher> <20221012205757.GA3118709@bhelgaas>
 <CAJZ5v0gWG1qVzwSy19SSqFmxL7NZRf1pkLR_buPoyCcj4f0FLw@mail.gmail.com> <Y0iEaxyO8Jj6tKSC@mail.local>
In-Reply-To: <Y0iEaxyO8Jj6tKSC@mail.local>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 18 Oct 2022 18:12:12 +0200
Message-ID: <CAJZ5v0iOEP=ukAnu7Rs84AfDx=ULKb2eF_0vV5qCb8=66KFPiw@mail.gmail.com>
Subject: Re: [PATCH] rtc: rtc-cmos: Fix event handler registration ordering issue
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Bjorn Helgaas <helgaas@kernel.org>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Alessandro Zummo <a.zummo@towertech.it>,
        Mario Limonciello <mario.limonciello@amd.com>,
        linux-rtc@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Linux ACPI <linux-acpi@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Mel Gorman <mgorman@techsingularity.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 13, 2022 at 11:34 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 13/10/2022 13:38:31+0200, Rafael J. Wysocki wrote:
> > On Wed, Oct 12, 2022 at 11:00 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Wed, Oct 12, 2022 at 08:07:01PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Because acpi_install_fixed_event_handler() enables the event
> > > > automatically on success, it is incorrect to call it before the
> > > > handler routine passed to it is ready to handle events.
> > > >
> > > > Unfortunately, the rtc-cmos driver does exactly the incorrect thing
> > > > by calling cmos_wake_setup(), which passes rtc_handler() to
> > > > acpi_install_fixed_event_handler(), before cmos_do_probe(), because
> > > > rtc_handler() uses dev_get_drvdata() to get to the cmos object
> > > > pointer and the driver data pointer is only populated in
> > > > cmos_do_probe().
> > > >
> > > > This leads to a NULL pointer dereference in rtc_handler() on boot
> > > > if the RTC fixed event happens to be active at the init time.
> > > >
> > > > To address this issue, change the initialization ordering of the
> > > > driver so that cmos_wake_setup() is always called after a successful
> > > > cmos_do_probe() call.
> > > >
> > > > While at it, change cmos_pnp_probe() to call cmos_do_probe() after
> > > > the initial if () statement used for computing the IRQ argument to
> > > > be passed to cmos_do_probe() which is cleaner than calling it in
> > > > each branch of that if () (local variable "irq" can be of type int,
> > > > because it is passed to that function as an argument of type int).
> > > >
> > > > Note that commit 6492fed7d8c9 ("rtc: rtc-cmos: Do not check
> > > > ACPI_FADT_LOW_POWER_S0") caused this issue to affect a larger number
> > > > of systems, because previously it only affected systems with
> > > > ACPI_FADT_LOW_POWER_S0 set, but it is present regardless of that
> > > > commit.
> > > >
> > > > Fixes: 6492fed7d8c9 ("rtc: rtc-cmos: Do not check ACPI_FADT_LOW_POWER_S0")
> > > > Fixes: a474aaedac99 ("rtc-cmos: move wake setup from ACPI glue into RTC driver")
> > > > Link: https://lore.kernel.org/linux-acpi/20221010141630.zfzi7mk7zvnmclzy@techsingularity.net/
> > > > Reported-by: Mel Gorman <mgorman@techsingularity.net>
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> > >
> > > Yep, I blew it with a474aaedac99, sorry about that.
> > >
> > > Possibly could call cmos_wake_setup() from cmos_do_probe() instead of
> > > from cmos_pnp_probe() and cmos_platform_probe()?
> >
> > Sounds good.
> >
> > I would prefer to send a separate patch for this on top of the
> > $subject one, unless Alexandre wants me to do it all in one go.
> >
> > Alexandre, what's your preference here?  Or would you prefer if I
> > pushed this forward?
> >
>
> I applied your patch, feel free to improve on top of that ;)

Thank you!

Unfortunately, I broke the wake alarm with this change and a fix has
just been posted:

https://lore.kernel.org/linux-acpi/5887691.lOV4Wx5bFT@kreacher/