Date:   Thu, 13 Oct 2022 13:38:31 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Bjorn Helgaas <helgaas@...nel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Mario Limonciello <mario.limonciello@....com>,
        linux-rtc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Mel Gorman <mgorman@...hsingularity.net>
Subject: Re: [PATCH] rtc: rtc-cmos: Fix event handler registration ordering issue

On Wed, Oct 12, 2022 at 11:00 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Wed, Oct 12, 2022 at 08:07:01PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...hsingularity.net>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Reviewed-by: Bjorn Helgaas <bhelgaas@...gle.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?

> Then there would be a single call site and it would be closer to the actual dependency on
> dev_set_drvdata().  Either way is fine with me.

OK

> Unrelated, but I happened to notice that pnp_irq() returns -1 for
> failure, and this note suggests that possibly returning 0 would be
> better:
>
>   https://lore.kernel.org/r/CAHk-=wg2Pkb9kbfbstbB91AJA2SF6cySbsgHG-iQMq56j3VTcA@mail.gmail.com

Probably.

In that case, though, it would be prudent to also explicitly discard
IRQ resources where start is equal to 0.

>
> > ---
> >  drivers/rtc/rtc-cmos.c |   29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > Index: linux-pm/drivers/rtc/rtc-cmos.c
> > ===================================================================
> > --- linux-pm.orig/drivers/rtc/rtc-cmos.c
> > +++ linux-pm/drivers/rtc/rtc-cmos.c
> > @@ -1352,10 +1352,10 @@ static void cmos_check_acpi_rtc_status(s
> >
> >  static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
> >  {
> > -     cmos_wake_setup(&pnp->dev);
> > +     int irq, ret;
> >
> >       if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) {
> > -             unsigned int irq = 0;
> > +             irq = 0;
> >  #ifdef CONFIG_X86
> >               /* Some machines contain a PNP entry for the RTC, but
> >                * don't define the IRQ. It should always be safe to
> > @@ -1364,13 +1364,17 @@ static int cmos_pnp_probe(struct pnp_dev
> >               if (nr_legacy_irqs())
> >                       irq = RTC_IRQ;
> >  #endif
> > -             return cmos_do_probe(&pnp->dev,
> > -                             pnp_get_resource(pnp, IORESOURCE_IO, 0), irq);
> >       } else {
> > -             return cmos_do_probe(&pnp->dev,
> > -                             pnp_get_resource(pnp, IORESOURCE_IO, 0),
> > -                             pnp_irq(pnp, 0));
> > +             irq = pnp_irq(pnp, 0);
> >       }
> > +
> > +     ret = cmos_do_probe(&pnp->dev, pnp_get_resource(pnp, IORESOURCE_IO, 0), irq);
> > +     if (ret)
> > +             return ret;
> > +
> > +     cmos_wake_setup(&pnp->dev);
> > +
> > +     return 0;
> >  }
> >
> >  static void cmos_pnp_remove(struct pnp_dev *pnp)
> > @@ -1454,10 +1458,9 @@ static inline void cmos_of_init(struct p
> >  static int __init cmos_platform_probe(struct platform_device *pdev)
> >  {
> >       struct resource *resource;
> > -     int irq;
> > +     int irq, ret;
> >
> >       cmos_of_init(pdev);
> > -     cmos_wake_setup(&pdev->dev);
> >
> >       if (RTC_IOMAPPED)
> >               resource = platform_get_resource(pdev, IORESOURCE_IO, 0);
> > @@ -1467,7 +1470,13 @@ static int __init cmos_platform_probe(st
> >       if (irq < 0)
> >               irq = -1;
> >
> > -     return cmos_do_probe(&pdev->dev, resource, irq);
> > +     ret = cmos_do_probe(&pdev->dev, resource, irq);
> > +     if (ret)
> > +             return ret;
> > +
> > +     cmos_wake_setup(&pdev->dev);
> > +
> > +     return 0;
> >  }
> >
> >  static int cmos_platform_remove(struct platform_device *pdev)
> >
> >
> >

Powered by blists - more mailing lists