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@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. 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
