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