linux-kernel - Re: [PATCH] ACPI: scan: Rearrange dep

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 11 May 2021 13:42:20 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ACPI: scan: Rearrange dep_unmet initialization

On Tue, May 11, 2021 at 1:24 PM Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi Rafael,
>
> On 5/10/21 7:53 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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@...hat.com>

Thank you!