Re: Missing default handler for the EmbeddedControl OpRegion

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

 



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):
> >>>>> 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().





[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