* Re: Missing default handler for the EmbeddedControl OpRegion
2024-05-08 12:50 ` Rafael J. Wysocki
@ 2024-05-08 12:55 ` Rafael J. Wysocki
2024-05-08 17:35 ` Mario Limonciello
2024-05-09 10:35 ` Hans de Goede
2 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2024-05-08 12:55 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Andy Shevchenko, Rafael J. Wysocki, Rafael J. Wysocki, Len Brown,
Robert Moore, Hans de Goede, Mario Limonciello, Linux ACPI
On Wed, May 8, 2024 at 2:50 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> [Resending because it appears to have got lost, sorry for duplicates.]
>
> On Wednesday, May 8, 2024 2:20:59 PM CEST Heikki Krogerus wrote:
> > On Mon, May 06, 2024 at 07:45:07PM +0200, Rafael J. Wysocki wrote:
> > > Hi,
> > >
> > > On Mon, Apr 29, 2024 at 4:55 PM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > There's a bug that is caused by an EmbeddedControl OpRegion which is
> > > > declared inside the scope of a specific USB Type-C device (PNP0CA0):
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=218789
> > >
> > > And in this bug you are essentially proposing to install the EC
> > > OpRegion handler at the namespace root instead of the EC device.
> > >
> > > This sounds reasonable, although AFAICS this is a matter of modifying
> > > the EC driver (before the EC OpRegion handler is installed by the EC
> > > drvier, ACPICA has no way to handle EC address space accesses anyway).
> > >
> > > > It looks like that's not the only case where that OpRegion ID is used
> > > > outside of the EC device scope. There is at least one driver in Linux
> > > > Kernel (drivers/platform/x86/wmi.c) that already has a custom handler
> > > > for the EmbeddedControl OpRegion, and based on a quick search, the
> > > > problem "Region EmbeddedControl (ID=3) has no handler" has happened
> > > > with some other devices too.
> > >
> > > AFAICS, installing the EC address space handler at the EC device
> > > object itself is not based on any sound technical arguments, it's just
> > > been always done this way in Linux. It's quite possible that the EC
> > > address space handler should have been installed at the namespace root
> > > from the outset.
> >
> > Okay, thank you for the explanation. So can we simply change it like
> > this (I may have still misunderstood something)?
>
> Roughly speaking, yes, but it is missing an analogous change around
> the removal.
Actually, not around the removal, but around the evaluation of _REG
methods and dropping address_space_handler_holder is better IMO,
because it will always be ACPI_ROOT_OBJECT now.
> Please see the appended patch (which I have created independently in
> the meantime). It doesn't break stuff for me and Andy points out that
> there are examples of EmbeddedControl OpRegions outside the EC device
> scope in the spec (see Section 9.17.15 in ACPI 6.5, for instance).
>
> So I think that this change can be made relatively safely (but adding Hans and
> Mario to the CC in case they know something that might be broken by it).
>
>
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index 02255795b800..6b9dd27171ee 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
> >
> > if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
> > acpi_ec_enter_noirq(ec);
> > - status = acpi_install_address_space_handler_no_reg(ec->handle,
> > + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
> > ACPI_ADR_SPACE_EC,
> > &acpi_ec_space_handler,
> > NULL, ec);
> > @@ -1497,7 +1497,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
> > return -ENODEV;
> > }
> > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> > - ec->address_space_handler_holder = ec->handle;
> > + ec->address_space_handler_holder = ACPI_ROOT_OBJECT;
> > }
> >
> > if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
> >
>
> ---
> drivers/acpi/ec.c | 10 +++++-----
> drivers/acpi/internal.h | 1 -
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct ac
>
> if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
> acpi_ec_enter_noirq(ec);
> - status = acpi_install_address_space_handler_no_reg(ec->handle,
> + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
> ACPI_ADR_SPACE_EC,
> &acpi_ec_space_handler,
> NULL, ec);
> @@ -1497,11 +1497,10 @@ static int ec_install_handlers(struct ac
> return -ENODEV;
> }
> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> - ec->address_space_handler_holder = ec->handle;
> }
>
> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
> - acpi_execute_reg_methods(ec->handle, ACPI_ADR_SPACE_EC);
> + acpi_execute_reg_methods(ACPI_ROOT_OBJECT, ACPI_ADR_SPACE_EC);
> set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags);
> }
>
> @@ -1555,8 +1554,9 @@ static void ec_remove_handlers(struct ac
> {
> if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
> if (ACPI_FAILURE(acpi_remove_address_space_handler(
> - ec->address_space_handler_holder,
> - ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
> + ACPI_ROOT_OBJECT,
> + ACPI_ADR_SPACE_EC,
> + &acpi_ec_space_handler)))
> pr_err("failed to remove space handler\n");
> clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> }
> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -186,7 +186,6 @@ enum acpi_ec_event_state {
>
> struct acpi_ec {
> acpi_handle handle;
> - acpi_handle address_space_handler_holder;
> int gpe;
> int irq;
> unsigned long command_addr;
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Missing default handler for the EmbeddedControl OpRegion
2024-05-08 12:50 ` Rafael J. Wysocki
2024-05-08 12:55 ` Rafael J. Wysocki
@ 2024-05-08 17:35 ` Mario Limonciello
2024-05-09 10:35 ` Hans de Goede
2 siblings, 0 replies; 12+ messages in thread
From: Mario Limonciello @ 2024-05-08 17:35 UTC (permalink / raw)
To: Rafael J. Wysocki, Heikki Krogerus
Cc: Andy Shevchenko, Rafael J. Wysocki, Len Brown, Robert Moore,
Hans de Goede, Linux ACPI
On 5/8/2024 07:50, Rafael J. Wysocki wrote:
> [Resending because it appears to have got lost, sorry for duplicates.]
>
> On Wednesday, May 8, 2024 2:20:59 PM CEST Heikki Krogerus wrote:
>> On Mon, May 06, 2024 at 07:45:07PM +0200, Rafael J. Wysocki wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2024 at 4:55 PM Heikki Krogerus
>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> There's a bug that is caused by an EmbeddedControl OpRegion which is
>>>> declared inside the scope of a specific USB Type-C device (PNP0CA0):
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=218789
>>>
>>> And in this bug you are essentially proposing to install the EC
>>> OpRegion handler at the namespace root instead of the EC device.
>>>
>>> This sounds reasonable, although AFAICS this is a matter of modifying
>>> the EC driver (before the EC OpRegion handler is installed by the EC
>>> drvier, ACPICA has no way to handle EC address space accesses anyway).
>>>
>>>> It looks like that's not the only case where that OpRegion ID is used
>>>> outside of the EC device scope. There is at least one driver in Linux
>>>> Kernel (drivers/platform/x86/wmi.c) that already has a custom handler
>>>> for the EmbeddedControl OpRegion, and based on a quick search, the
>>>> problem "Region EmbeddedControl (ID=3) has no handler" has happened
>>>> with some other devices too.
>>>
>>> AFAICS, installing the EC address space handler at the EC device
>>> object itself is not based on any sound technical arguments, it's just
>>> been always done this way in Linux. It's quite possible that the EC
>>> address space handler should have been installed at the namespace root
>>> from the outset.
>>
>> Okay, thank you for the explanation. So can we simply change it like
>> this (I may have still misunderstood something)?
>
> Roughly speaking, yes, but it is missing an analogous change around
> the removal.
>
> Please see the appended patch (which I have created independently in
> the meantime). It doesn't break stuff for me and Andy points out that
> there are examples of EmbeddedControl OpRegions outside the EC device
> scope in the spec (see Section 9.17.15 in ACPI 6.5, for instance).
>
> So I think that this change can be made relatively safely (but adding Hans and
> Mario to the CC in case they know something that might be broken by it).
Nothing jumps out at me, it looks like a good way to handle this issue.
>
>
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index 02255795b800..6b9dd27171ee 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>>
>> if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
>> acpi_ec_enter_noirq(ec);
>> - status = acpi_install_address_space_handler_no_reg(ec->handle,
>> + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
>> ACPI_ADR_SPACE_EC,
>> &acpi_ec_space_handler,
>> NULL, ec);
>> @@ -1497,7 +1497,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>> return -ENODEV;
>> }
>> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>> - ec->address_space_handler_holder = ec->handle;
>> + ec->address_space_handler_holder = ACPI_ROOT_OBJECT;
>> }
>>
>> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
>>
>
> ---
> drivers/acpi/ec.c | 10 +++++-----
> drivers/acpi/internal.h | 1 -
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct ac
>
> if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
> acpi_ec_enter_noirq(ec);
> - status = acpi_install_address_space_handler_no_reg(ec->handle,
> + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
> ACPI_ADR_SPACE_EC,
> &acpi_ec_space_handler,
> NULL, ec);
> @@ -1497,11 +1497,10 @@ static int ec_install_handlers(struct ac
> return -ENODEV;
> }
> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> - ec->address_space_handler_holder = ec->handle;
> }
>
> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
> - acpi_execute_reg_methods(ec->handle, ACPI_ADR_SPACE_EC);
> + acpi_execute_reg_methods(ACPI_ROOT_OBJECT, ACPI_ADR_SPACE_EC);
> set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags);
> }
>
> @@ -1555,8 +1554,9 @@ static void ec_remove_handlers(struct ac
> {
> if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
> if (ACPI_FAILURE(acpi_remove_address_space_handler(
> - ec->address_space_handler_holder,
> - ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
> + ACPI_ROOT_OBJECT,
> + ACPI_ADR_SPACE_EC,
> + &acpi_ec_space_handler)))
> pr_err("failed to remove space handler\n");
> clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> }
> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -186,7 +186,6 @@ enum acpi_ec_event_state {
>
> struct acpi_ec {
> acpi_handle handle;
> - acpi_handle address_space_handler_holder;
> int gpe;
> int irq;
> unsigned long command_addr;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Missing default handler for the EmbeddedControl OpRegion
2024-05-08 12:50 ` Rafael J. Wysocki
2024-05-08 12:55 ` Rafael J. Wysocki
2024-05-08 17:35 ` Mario Limonciello
@ 2024-05-09 10:35 ` Hans de Goede
2024-05-09 22:31 ` Armin Wolf
2 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2024-05-09 10:35 UTC (permalink / raw)
To: Rafael J. Wysocki, Heikki Krogerus, Armin Wolf
Cc: Andy Shevchenko, Rafael J. Wysocki, Len Brown, Robert Moore,
Mario Limonciello, Linux ACPI
Hi Rafael,
On 5/8/24 2:50 PM, Rafael J. Wysocki wrote:
> [Resending because it appears to have got lost, sorry for duplicates.]
>
> On Wednesday, May 8, 2024 2:20:59 PM CEST Heikki Krogerus wrote:
>> On Mon, May 06, 2024 at 07:45:07PM +0200, Rafael J. Wysocki wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2024 at 4:55 PM Heikki Krogerus
>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> There's a bug that is caused by an EmbeddedControl OpRegion which is
>>>> declared inside the scope of a specific USB Type-C device (PNP0CA0):
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=218789
>>>
>>> And in this bug you are essentially proposing to install the EC
>>> OpRegion handler at the namespace root instead of the EC device.
>>>
>>> This sounds reasonable, although AFAICS this is a matter of modifying
>>> the EC driver (before the EC OpRegion handler is installed by the EC
>>> drvier, ACPICA has no way to handle EC address space accesses anyway).
>>>
>>>> It looks like that's not the only case where that OpRegion ID is used
>>>> outside of the EC device scope. There is at least one driver in Linux
>>>> Kernel (drivers/platform/x86/wmi.c) that already has a custom handler
>>>> for the EmbeddedControl OpRegion, and based on a quick search, the
>>>> problem "Region EmbeddedControl (ID=3) has no handler" has happened
>>>> with some other devices too.
>>>
>>> AFAICS, installing the EC address space handler at the EC device
>>> object itself is not based on any sound technical arguments, it's just
>>> been always done this way in Linux. It's quite possible that the EC
>>> address space handler should have been installed at the namespace root
>>> from the outset.
>>
>> Okay, thank you for the explanation. So can we simply change it like
>> this (I may have still misunderstood something)?
>
> Roughly speaking, yes, but it is missing an analogous change around
> the removal.
>
> Please see the appended patch (which I have created independently in
> the meantime). It doesn't break stuff for me and Andy points out that
> there are examples of EmbeddedControl OpRegions outside the EC device
> scope in the spec (see Section 9.17.15 in ACPI 6.5, for instance).
>
> So I think that this change can be made relatively safely (but adding Hans and
> Mario to the CC in case they know something that might be broken by it).
+Cc Armin for WMI EC handler
No objections from me against registering the EC handler at the root,
when I saw that the WMI driver was registering its own handler I was
already wondering why we did not just register the acpi/ec.c handler at
the root level but I did not have time to pursue this further.
One question which I have is does the drivers/acpi/ec.c version handle
read/writes of a width other then 8 bits ? Armin recently added support
for other widths to the WMI version of the OpRegion handler to fix
issues on some laptop models:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c663b26972eae7d2a614f584c92a266fe9a2d44c
Regards,
Hans
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index 02255795b800..6b9dd27171ee 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>>
>> if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
>> acpi_ec_enter_noirq(ec);
>> - status = acpi_install_address_space_handler_no_reg(ec->handle,
>> + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
>> ACPI_ADR_SPACE_EC,
>> &acpi_ec_space_handler,
>> NULL, ec);
>> @@ -1497,7 +1497,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>> return -ENODEV;
>> }
>> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>> - ec->address_space_handler_holder = ec->handle;
>> + ec->address_space_handler_holder = ACPI_ROOT_OBJECT;
>> }
>>
>> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
>>
>
> ---
> drivers/acpi/ec.c | 10 +++++-----
> drivers/acpi/internal.h | 1 -
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct ac
>
> if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
> acpi_ec_enter_noirq(ec);
> - status = acpi_install_address_space_handler_no_reg(ec->handle,
> + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
> ACPI_ADR_SPACE_EC,
> &acpi_ec_space_handler,
> NULL, ec);
> @@ -1497,11 +1497,10 @@ static int ec_install_handlers(struct ac
> return -ENODEV;
> }
> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> - ec->address_space_handler_holder = ec->handle;
> }
>
> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
> - acpi_execute_reg_methods(ec->handle, ACPI_ADR_SPACE_EC);
> + acpi_execute_reg_methods(ACPI_ROOT_OBJECT, ACPI_ADR_SPACE_EC);
> set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags);
> }
>
> @@ -1555,8 +1554,9 @@ static void ec_remove_handlers(struct ac
> {
> if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
> if (ACPI_FAILURE(acpi_remove_address_space_handler(
> - ec->address_space_handler_holder,
> - ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
> + ACPI_ROOT_OBJECT,
> + ACPI_ADR_SPACE_EC,
> + &acpi_ec_space_handler)))
> pr_err("failed to remove space handler\n");
> clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> }
> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -186,7 +186,6 @@ enum acpi_ec_event_state {
>
> struct acpi_ec {
> acpi_handle handle;
> - acpi_handle address_space_handler_holder;
> int gpe;
> int irq;
> unsigned long command_addr;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Missing default handler for the EmbeddedControl OpRegion
2024-05-09 10:35 ` Hans de Goede
@ 2024-05-09 22:31 ` Armin Wolf
2024-05-10 10:03 ` Rafael J. Wysocki
0 siblings, 1 reply; 12+ messages in thread
From: Armin Wolf @ 2024-05-09 22:31 UTC (permalink / raw)
To: Hans de Goede, Rafael J. Wysocki, Heikki Krogerus
Cc: Andy Shevchenko, Rafael J. Wysocki, Len Brown, Robert Moore,
Mario Limonciello, Linux ACPI
Am 09.05.24 um 12:35 schrieb Hans de Goede:
> Hi Rafael,
>
> On 5/8/24 2:50 PM, Rafael J. Wysocki wrote:
>> [Resending because it appears to have got lost, sorry for duplicates.]
>>
>> On Wednesday, May 8, 2024 2:20:59 PM CEST Heikki Krogerus wrote:
>>> On Mon, May 06, 2024 at 07:45:07PM +0200, Rafael J. Wysocki wrote:
>>>> Hi,
>>>>
>>>> On Mon, Apr 29, 2024 at 4:55 PM Heikki Krogerus
>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>> Hi,
>>>>>
>>>>> There's a bug that is caused by an EmbeddedControl OpRegion which is
>>>>> declared inside the scope of a specific USB Type-C device (PNP0CA0):
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=218789
>>>> And in this bug you are essentially proposing to install the EC
>>>> OpRegion handler at the namespace root instead of the EC device.
>>>>
>>>> This sounds reasonable, although AFAICS this is a matter of modifying
>>>> the EC driver (before the EC OpRegion handler is installed by the EC
>>>> drvier, ACPICA has no way to handle EC address space accesses anyway).
>>>>
>>>>> It looks like that's not the only case where that OpRegion ID is used
>>>>> outside of the EC device scope. There is at least one driver in Linux
>>>>> Kernel (drivers/platform/x86/wmi.c) that already has a custom handler
>>>>> for the EmbeddedControl OpRegion, and based on a quick search, the
>>>>> problem "Region EmbeddedControl (ID=3) has no handler" has happened
>>>>> with some other devices too.
>>>> AFAICS, installing the EC address space handler at the EC device
>>>> object itself is not based on any sound technical arguments, it's just
>>>> been always done this way in Linux. It's quite possible that the EC
>>>> address space handler should have been installed at the namespace root
>>>> from the outset.
>>> Okay, thank you for the explanation. So can we simply change it like
>>> this (I may have still misunderstood something)?
>> Roughly speaking, yes, but it is missing an analogous change around
>> the removal.
>>
>> Please see the appended patch (which I have created independently in
>> the meantime). It doesn't break stuff for me and Andy points out that
>> there are examples of EmbeddedControl OpRegions outside the EC device
>> scope in the spec (see Section 9.17.15 in ACPI 6.5, for instance).
>>
>> So I think that this change can be made relatively safely (but adding Hans and
>> Mario to the CC in case they know something that might be broken by it).
> +Cc Armin for WMI EC handler
>
> No objections from me against registering the EC handler at the root,
> when I saw that the WMI driver was registering its own handler I was
> already wondering why we did not just register the acpi/ec.c handler at
> the root level but I did not have time to pursue this further.
>
> One question which I have is does the drivers/acpi/ec.c version handle
> read/writes of a width other then 8 bits ? Armin recently added support
> for other widths to the WMI version of the OpRegion handler to fix
> issues on some laptop models:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c663b26972eae7d2a614f584c92a266fe9a2d44c
>
> Regards,
>
> Hans
Hi,
the handling of multi-byte reads/writes was taken from the ec driver itself, so
using the standard ec handler should make no difference for the WMI driver.
Thanks,
Armin Wolf
>>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>>> index 02255795b800..6b9dd27171ee 100644
>>> --- a/drivers/acpi/ec.c
>>> +++ b/drivers/acpi/ec.c
>>> @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>>>
>>> if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
>>> acpi_ec_enter_noirq(ec);
>>> - status = acpi_install_address_space_handler_no_reg(ec->handle,
>>> + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
>>> ACPI_ADR_SPACE_EC,
>>> &acpi_ec_space_handler,
>>> NULL, ec);
>>> @@ -1497,7 +1497,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>>> return -ENODEV;
>>> }
>>> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>>> - ec->address_space_handler_holder = ec->handle;
>>> + ec->address_space_handler_holder = ACPI_ROOT_OBJECT;
>>> }
>>>
>>> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
>>>
>> ---
>> drivers/acpi/ec.c | 10 +++++-----
>> drivers/acpi/internal.h | 1 -
>> 2 files changed, 5 insertions(+), 6 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/ec.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/ec.c
>> +++ linux-pm/drivers/acpi/ec.c
>> @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct ac
>>
>> if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
>> acpi_ec_enter_noirq(ec);
>> - status = acpi_install_address_space_handler_no_reg(ec->handle,
>> + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT,
>> ACPI_ADR_SPACE_EC,
>> &acpi_ec_space_handler,
>> NULL, ec);
>> @@ -1497,11 +1497,10 @@ static int ec_install_handlers(struct ac
>> return -ENODEV;
>> }
>> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>> - ec->address_space_handler_holder = ec->handle;
>> }
>>
>> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
>> - acpi_execute_reg_methods(ec->handle, ACPI_ADR_SPACE_EC);
>> + acpi_execute_reg_methods(ACPI_ROOT_OBJECT, ACPI_ADR_SPACE_EC);
>> set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags);
>> }
>>
>> @@ -1555,8 +1554,9 @@ static void ec_remove_handlers(struct ac
>> {
>> if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
>> if (ACPI_FAILURE(acpi_remove_address_space_handler(
>> - ec->address_space_handler_holder,
>> - ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
>> + ACPI_ROOT_OBJECT,
>> + ACPI_ADR_SPACE_EC,
>> + &acpi_ec_space_handler)))
>> pr_err("failed to remove space handler\n");
>> clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>> }
>> Index: linux-pm/drivers/acpi/internal.h
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/internal.h
>> +++ linux-pm/drivers/acpi/internal.h
>> @@ -186,7 +186,6 @@ enum acpi_ec_event_state {
>>
>> struct acpi_ec {
>> acpi_handle handle;
>> - acpi_handle address_space_handler_holder;
>> int gpe;
>> int irq;
>> unsigned long command_addr;
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Missing default handler for the EmbeddedControl OpRegion
2024-05-09 22:31 ` Armin Wolf
@ 2024-05-10 10:03 ` Rafael J. Wysocki
2024-05-11 13:20 ` Hans de Goede
0 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2024-05-10 10:03 UTC (permalink / raw)
To: Armin Wolf
Cc: Hans de Goede, Rafael J. Wysocki, Heikki Krogerus,
Andy Shevchenko, Rafael J. Wysocki, Len Brown, Robert Moore,
Mario Limonciello, Linux ACPI
On Fri, May 10, 2024 at 12:32 AM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Am 09.05.24 um 12:35 schrieb Hans de Goede:
>
> > Hi Rafael,
> >
> > On 5/8/24 2:50 PM, Rafael J. Wysocki wrote:
> >> [Resending because it appears to have got lost, sorry for duplicates.]
> >>
> >> On Wednesday, May 8, 2024 2:20:59 PM CEST Heikki Krogerus wrote:
> >>> On Mon, May 06, 2024 at 07:45:07PM +0200, Rafael J. Wysocki wrote:
> >>>> Hi,
> >>>>
> >>>> On Mon, Apr 29, 2024 at 4:55 PM Heikki Krogerus
> >>>> <heikki.krogerus@linux.intel.com> wrote:
> >>>>> Hi,
> >>>>>
> >>>>> There's a bug that is caused by an EmbeddedControl OpRegion which is
> >>>>> declared inside the scope of a specific USB Type-C device (PNP0CA0):
> >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=218789
> >>>> And in this bug you are essentially proposing to install the EC
> >>>> OpRegion handler at the namespace root instead of the EC device.
> >>>>
> >>>> This sounds reasonable, although AFAICS this is a matter of modifying
> >>>> the EC driver (before the EC OpRegion handler is installed by the EC
> >>>> drvier, ACPICA has no way to handle EC address space accesses anyway).
> >>>>
> >>>>> It looks like that's not the only case where that OpRegion ID is used
> >>>>> outside of the EC device scope. There is at least one driver in Linux
> >>>>> Kernel (drivers/platform/x86/wmi.c) that already has a custom handler
> >>>>> for the EmbeddedControl OpRegion, and based on a quick search, the
> >>>>> problem "Region EmbeddedControl (ID=3) has no handler" has happened
> >>>>> with some other devices too.
> >>>> AFAICS, installing the EC address space handler at the EC device
> >>>> object itself is not based on any sound technical arguments, it's just
> >>>> been always done this way in Linux. It's quite possible that the EC
> >>>> address space handler should have been installed at the namespace root
> >>>> from the outset.
> >>> Okay, thank you for the explanation. So can we simply change it like
> >>> this (I may have still misunderstood something)?
> >> Roughly speaking, yes, but it is missing an analogous change around
> >> the removal.
> >>
> >> Please see the appended patch (which I have created independently in
> >> the meantime). It doesn't break stuff for me and Andy points out that
> >> there are examples of EmbeddedControl OpRegions outside the EC device
> >> scope in the spec (see Section 9.17.15 in ACPI 6.5, for instance).
> >>
> >> So I think that this change can be made relatively safely (but adding Hans and
> >> Mario to the CC in case they know something that might be broken by it).
> > +Cc Armin for WMI EC handler
> >
> > No objections from me against registering the EC handler at the root,
> > when I saw that the WMI driver was registering its own handler I was
> > already wondering why we did not just register the acpi/ec.c handler at
> > the root level but I did not have time to pursue this further.
> >
> > One question which I have is does the drivers/acpi/ec.c version handle
> > read/writes of a width other then 8 bits ? Armin recently added support
> > for other widths to the WMI version of the OpRegion handler to fix
> > issues on some laptop models:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c663b26972eae7d2a614f584c92a266fe9a2d44c
> >
> > Regards,
> >
> > Hans
>
> Hi,
>
> the handling of multi-byte reads/writes was taken from the ec driver itself, so
> using the standard ec handler should make no difference for the WMI driver.
Thanks for the confirmation!
So AFAICS acpi_wmi_ec_space_handler() will not be necessary after this
change, but so long as it is installed by acpi_wmi_probe(), it will be
used for opregions in the WMI device scope, so I'd prefer to remove it
in a separate patch to avoid making too many changes in one go.
Let me add a changelog to the patch posted here
https://lore.kernel.org/linux-acpi/5781917.DvuYhMxLoT@kreacher/
and submit it properly along with a separate change removing
acpi_wmi_ec_space_handler().
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Missing default handler for the EmbeddedControl OpRegion
2024-05-10 10:03 ` Rafael J. Wysocki
@ 2024-05-11 13:20 ` Hans de Goede
0 siblings, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2024-05-11 13:20 UTC (permalink / raw)
To: Rafael J. Wysocki, Armin Wolf
Cc: Rafael J. Wysocki, Heikki Krogerus, Andy Shevchenko, Len Brown,
Robert Moore, Mario Limonciello, Linux ACPI
Hi,
On 5/10/24 12:03 PM, Rafael J. Wysocki wrote:
> On Fri, May 10, 2024 at 12:32 AM Armin Wolf <W_Armin@gmx.de> wrote:
>>
>> Am 09.05.24 um 12:35 schrieb Hans de Goede:
>>
>>> Hi Rafael,
>>>
>>> On 5/8/24 2:50 PM, Rafael J. Wysocki wrote:
>>>> [Resending because it appears to have got lost, sorry for duplicates.]
>>>>
>>>> On Wednesday, May 8, 2024 2:20:59 PM CEST Heikki Krogerus wrote:
>>>>> On Mon, May 06, 2024 at 07:45:07PM +0200, Rafael J. Wysocki wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Apr 29, 2024 at 4:55 PM Heikki Krogerus
>>>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> There's a bug that is caused by an EmbeddedControl OpRegion which is
>>>>>>> declared inside the scope of a specific USB Type-C device (PNP0CA0):
>>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=218789
>>>>>> And in this bug you are essentially proposing to install the EC
>>>>>> OpRegion handler at the namespace root instead of the EC device.
>>>>>>
>>>>>> This sounds reasonable, although AFAICS this is a matter of modifying
>>>>>> the EC driver (before the EC OpRegion handler is installed by the EC
>>>>>> drvier, ACPICA has no way to handle EC address space accesses anyway).
>>>>>>
>>>>>>> It looks like that's not the only case where that OpRegion ID is used
>>>>>>> outside of the EC device scope. There is at least one driver in Linux
>>>>>>> Kernel (drivers/platform/x86/wmi.c) that already has a custom handler
>>>>>>> for the EmbeddedControl OpRegion, and based on a quick search, the
>>>>>>> problem "Region EmbeddedControl (ID=3) has no handler" has happened
>>>>>>> with some other devices too.
>>>>>> AFAICS, installing the EC address space handler at the EC device
>>>>>> object itself is not based on any sound technical arguments, it's just
>>>>>> been always done this way in Linux. It's quite possible that the EC
>>>>>> address space handler should have been installed at the namespace root
>>>>>> from the outset.
>>>>> Okay, thank you for the explanation. So can we simply change it like
>>>>> this (I may have still misunderstood something)?
>>>> Roughly speaking, yes, but it is missing an analogous change around
>>>> the removal.
>>>>
>>>> Please see the appended patch (which I have created independently in
>>>> the meantime). It doesn't break stuff for me and Andy points out that
>>>> there are examples of EmbeddedControl OpRegions outside the EC device
>>>> scope in the spec (see Section 9.17.15 in ACPI 6.5, for instance).
>>>>
>>>> So I think that this change can be made relatively safely (but adding Hans and
>>>> Mario to the CC in case they know something that might be broken by it).
>>> +Cc Armin for WMI EC handler
>>>
>>> No objections from me against registering the EC handler at the root,
>>> when I saw that the WMI driver was registering its own handler I was
>>> already wondering why we did not just register the acpi/ec.c handler at
>>> the root level but I did not have time to pursue this further.
>>>
>>> One question which I have is does the drivers/acpi/ec.c version handle
>>> read/writes of a width other then 8 bits ? Armin recently added support
>>> for other widths to the WMI version of the OpRegion handler to fix
>>> issues on some laptop models:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c663b26972eae7d2a614f584c92a266fe9a2d44c
>>>
>>> Regards,
>>>
>>> Hans
>>
>> Hi,
>>
>> the handling of multi-byte reads/writes was taken from the ec driver itself, so
>> using the standard ec handler should make no difference for the WMI driver.
>
> Thanks for the confirmation!
>
> So AFAICS acpi_wmi_ec_space_handler() will not be necessary after this
> change, but so long as it is installed by acpi_wmi_probe(), it will be
> used for opregions in the WMI device scope, so I'd prefer to remove it
> in a separate patch to avoid making too many changes in one go.
Ack.
> Let me add a changelog to the patch posted here
>
> https://lore.kernel.org/linux-acpi/5781917.DvuYhMxLoT@kreacher/
>
> and submit it properly along with a separate change removing
> acpi_wmi_ec_space_handler().
This sounds good to me.
Regards,
Hans
^ permalink raw reply [flat|nested] 12+ messages in thread