Re: [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	 LKML <linux-kernel@vger.kernel.org>,
	 Mika Westerberg <mika.westerberg@linux.intel.com>,
	 "Russell King (Oracle)" <linux@armlinux.org.uk>
Subject: Re: [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling
Date: Mon, 26 Feb 2024 16:54:27 +0100	[thread overview]
Message-ID: <CAJZ5v0jjD=KN0pOuWZZ8DT5yHdu03KgOSHYe3wB7h2vafNa44w@mail.gmail.com> (raw)
In-Reply-To: <CAJZ5v0id7ktjAne4wyEWox_xqjH9K=Kzbs3+Bcn1qHBctnincw@mail.gmail.com>

On Mon, Feb 26, 2024 at 4:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 22, 2024 at 7:28 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 21 Feb 2024 21:02:33 +0100
> > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

[...]

> > > +bool acpi_device_is_enabled(const struct acpi_device *adev)
> > > +{
> > > +     return acpi_device_is_present(adev) && adev->status.enabled;
> >
> > This resolves as (present or functional) && enabled.
> >
> > By my reading you are not allowed functional && enabled, but not present.
> > Line one of the description says.
> >
> > "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)."
> >
> > I don't much care about that though (I think we discussed this
> > in Russel's earlier series)
>
> Functional and enabled, but not present would go against the spec.  I
> guess the kernel could protect itself against this, but then whatever
> it chooses to do has not been defined anyway.
>
> The spec doesn't actually say what the OSPM is supposed to do when it
> sees that combination of bits.  I'm inclined to clarify it to say "if
> bit [0] is cleared, bit [1] has no defined meaning and it should be
> ignored by the OSPM".  To be consistent with this interpretation,
> acpi_device_is_enabled() should return "(present and enabled) or
> functional".
>
> I'll change it along these lines.

Actually, I don't think that the "functional" bit has any bearing on
this.  It only means that the OSPM should continue the enumeration
below the device regardless of the present bit value.

In the acpi_processor_add() case it is clearly irrelevant.

acpi_scan_bus_check() needs to walk the entire subtree below the
target device anyway.

As for acpi_scan_device_check(), IMO it's better to make it walk the
subtree below the device if it is present, but not enabled, for
backwards compatibility.