Re: Missing default handler for the EmbeddedControl OpRegion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



On 5/10/24 12:03 PM, Rafael J. Wysocki wrote:
> On Fri, May 10, 2024 at 12:32 AM Armin Wolf <W_Armin@xxxxxx> 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@xxxxxxxxxxxxxxx> 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):
>>>>>> 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:
>>> 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
> and submit it properly along with a separate change removing
> acpi_wmi_ec_space_handler().

This sounds good to me.



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux