Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <2607573.mvXUDI8C0e@kreacher> <44ec7350-38ee-b880-bcaf-fba5ddc288e9@redhat.com>
In-Reply-To: <44ec7350-38ee-b880-bcaf-fba5ddc288e9@redhat.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 11 May 2021 13:42:20 +0200
Message-ID: <CAJZ5v0jSDq8nZEeFUZc3arHHqWhUjDfmwPrJ69U=aO5-FB_7iA@mail.gmail.com>
Subject: Re: [PATCH] ACPI: scan: Rearrange dep_unmet initialization
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux ACPI <linux-acpi@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 11, 2021 at 1:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Rafael,
>
> On 5/10/21 7:53 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The dep_unmet field in struct acpi_device is used to store the
> > number of unresolved _DEP dependencies (that is, operation region
> > dependencies for which there are no drivers present) for the ACPI
> > device object represented by it.
> >
> > That field is initialized to 1 for all ACPI device objects in
> > acpi_add_single_object(), via acpi_init_device_object(), so as to
> > avoid evaluating _STA prematurely for battery device objects in
> > acpi_scan_init_status(), and it is "fixed up" in acpi_bus_check_add()
> > after the acpi_add_single_object() called by it has returned.
> >
> > This is not particularly straightforward and causes dep_unmet to
> > remain 1 for device objects without dependencies created by invoking
> > acpi_add_single_object() directly, outside acpi_bus_check_add().
> >
> > For this reason, rearrange acpi_add_single_object() to initialize
> > dep_unmet completely before calling acpi_scan_init_status(), which
> > requires passing one extra bool argument to it, and update all of
> > its callers accordingly.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Thanks, one small nitpick below.
>
> > ---
> >  drivers/acpi/scan.c |   62 +++++++++++++++++++++++++---------------------------
> >  1 file changed, 30 insertions(+), 32 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -1670,8 +1670,22 @@ void acpi_init_device_object(struct acpi
> >       device_initialize(&device->dev);
> >       dev_set_uevent_suppress(&device->dev, true);
> >       acpi_init_coherency(device);
> > -     /* Assume there are unmet deps to start with. */
> > -     device->dep_unmet = 1;
> > +}
> > +
> > +static void acpi_scan_dep_init(struct acpi_device *adev)
> > +{
> > +     struct acpi_dep_data *dep;
> > +
> > +     adev->dep_unmet = 0;
>
> Now that we don't set dep_unmet to 1 in acpi_init_device_object()
> anymore this line is no longer necessary.

Right.

I'll drop it before applying the patch.

> dep_unmet is set to 0 by the kzalloc of the adev and we are already
> relying on that in the case where the dep_init parameter to
> acpi_add_single_object() is false.
>
> But if you want to keep this that is fine too, either way
> this patch looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thank you!