Re: [RFC v3 4/4] ACPI: EC: fix ECDT probe ordering issues

From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andy@kernel.org>
Cc: "Rafael J . Wysocki" <rafael@kernel.org>,
	"Len Brown" <lenb@kernel.org>,
	"Robert Moore" <robert.moore@intel.com>,
	"Mika Westerberg" <mika.westerberg@linux.intel.com>,
	"Zhang Rui" <rui.zhang@intel.com>,
	kai.heng.feng@canonical.com,
	"Johannes Penßel" <johannespenssel@posteo.net>,
	linux-acpi@vger.kernel.org, devel@acpica.org
Subject: Re: [RFC v3 4/4] ACPI: EC: fix ECDT probe ordering issues
Date: Tue, 4 Oct 2022 11:23:30 +0200	[thread overview]
Message-ID: <9c3a138d-24c4-1bd1-0ec9-489302470f2f@redhat.com> (raw)
In-Reply-To: <YzvtwCTShC/yYovH@smile.fi.intel.com>

Hi,

On 10/4/22 10:24, Andy Shevchenko wrote:
> On Mon, Oct 03, 2022 at 04:42:14PM +0200, Hans de Goede wrote:
>> ACPI-2.0 says that the EC OpRegion handler must be available immediately
>> (like the standard default OpRegion handlers):
>>
>> Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ...
> 
> (Side note: As of today the revision 6.5 of the ACPI specification is
>  available)
> 
>> 2. OSPM must make Embedded Controller operation regions, accessed via
>> the Embedded Controllers described in ECDT, available before executing
>> any control method. These operation regions may become inaccessible
>> after OSPM runs _REG(EmbeddedControl, 0)."
>>
>> So acpi_bus_init() calls acpi_ec_ecdt_probe(), which calls
>> acpi_install_address_space_handler() to install the EC's OpRegion
>> handler, early on.
>>
>> This not only installs the OpRegion handler, but also calls the EC's
>> _REG method. The _REG method call is a problem because it may rely on
>> initialization done by the _INI methods of one of the PCI / _SB root devs,
>> see for example: https://bugzilla.kernel.org/show_bug.cgi?id=214899 .
>>
>> Generally speaking _REG methods are executed when the ACPI-device they
>> are part of has a driver bound to it. Where as _INI methods must be
>> executed at table load time (according to the spec). The problem here
>> is that the early acpi_install_address_space_handler() call causes
>> the _REG handler to run too early.
>>
>> To allow fixing this the ACPICA code now allows to split the OpRegion
>> handler installation and the executing of _REG into 2 separate steps.
>>
>> This commit uses this ACPICA functionality to fix the EC probe ordering
>> by delaying the executing of _REG for ECDT described ECs till the matching
>> EC device in the DSDT gets parsed and acpi_ec_add() for it gets called.
>> This moves the calling of _REG for the EC on devices with an ECDT to
>> the same point in time where it is called on devices without an ECDT table.
> 
> ...
> 
>> +	if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
>> +		acpi_execute_reg_methods(ec->handle, ACPI_ADR_SPACE_EC);
> 
>> +		set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags);
> 
> Just to be sure: do you need atomic operation here? Does it prevent of any
> kind of races? Because if it had been the case, it would have been done via
> test_and_set_bit() rather than testing and setting separated.

AFAIK we do not need atomic operations here (I did not verify this). Also note
that existing code a couple of lines above already uses the same pattern.

Regards,

Hans