| Message ID | 1514988151-12248-1-git-send-email-eric.auger@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module | expand |
On 1/3/2018 9:02 AM, Eric Auger wrote: > If I2C is built as a module, ACPI_I2C_OPREGION cannot be set > and any ACPI opregion calls targeting I2C fail with no opregion found. > > This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is > built into the kernel or built as a module. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> Hoping this makes to 4.16. Tested-by: Sinan Kaya <okaya@codeaurora.org>
On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: > If I2C is built as a module, ACPI_I2C_OPREGION cannot be set > and any ACPI opregion calls targeting I2C fail with no opregion found. > > This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is > built into the kernel or built as a module. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> I recall that we had some discussion until ending up with the current solution. And I finally found it again: http://www.serverphorums.com/read.php?12,1001402 In any case, I surely want Mika's ack on any change to ACPI related Kconfig symbols. Adding him to CC...
On Wed, Jan 24, 2018 at 06:56:16AM +0100, Wolfram Sang wrote: > On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: > > If I2C is built as a module, ACPI_I2C_OPREGION cannot be set > > and any ACPI opregion calls targeting I2C fail with no opregion found. > > > > This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is > > built into the kernel or built as a module. > > > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > I recall that we had some discussion until ending up with the current > solution. And I finally found it again: > > http://www.serverphorums.com/read.php?12,1001402 > > In any case, I surely want Mika's ack on any change to ACPI related > Kconfig symbols. Adding him to CC... So the problem is/was that what happens if you are in a middle of BIOS AML code touching the opregion and someone unloads the opregion handler? If you can quarantee nothing bad happens, then I'm fine with the patch :)
+linux-acpi On 1/24/2018 1:27 AM, Mika Westerberg wrote: > On Wed, Jan 24, 2018 at 06:56:16AM +0100, Wolfram Sang wrote: >> On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: >>> If I2C is built as a module, ACPI_I2C_OPREGION cannot be set >>> and any ACPI opregion calls targeting I2C fail with no opregion found. >>> >>> This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is >>> built into the kernel or built as a module. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> I recall that we had some discussion until ending up with the current >> solution. And I finally found it again: >> >> http://www.serverphorums.com/read.php?12,1001402 >> >> In any case, I surely want Mika's ack on any change to ACPI related >> Kconfig symbols. Adding him to CC... > > So the problem is/was that what happens if you are in a middle of BIOS > AML code touching the opregion and someone unloads the opregion handler? > If you can quarantee nothing bad happens, then I'm fine with the patch :) > Rafael to correct me if I got this right. The behavior of the operating system is well defined in the ACPI specification. Here is what I tested recently: ACPI defines _REG method to inform firmware of presence/removal of an operating region. When driver gets loaded, ACPI calls the _REG method with 1 argument. When driver gets unloaded, ACPI call the _REG method with 0 argument. Firmware can use this notification to its advantage to determine when an I2C related functionality should be accessed or not. If firmware doesn't use the _REG method, ACPI defines that AML statements accessing the operating region are ignored. You'll also see a warning from ACPICA saying the OperatingRegion 9 is no longer accessible and AML code execution failed. Also note that someone can always unbind an I2C driver from ACPI even with built-in module. I think we are talking about an orthogonal issue here.
On Wed, Jan 24, 2018 at 08:29:44AM -0500, Sinan Kaya wrote: > +linux-acpi > > On 1/24/2018 1:27 AM, Mika Westerberg wrote: > > On Wed, Jan 24, 2018 at 06:56:16AM +0100, Wolfram Sang wrote: > >> On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: > >>> If I2C is built as a module, ACPI_I2C_OPREGION cannot be set > >>> and any ACPI opregion calls targeting I2C fail with no opregion found. > >>> > >>> This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is > >>> built into the kernel or built as a module. > >>> > >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >> > >> I recall that we had some discussion until ending up with the current > >> solution. And I finally found it again: > >> > >> http://www.serverphorums.com/read.php?12,1001402 > >> > >> In any case, I surely want Mika's ack on any change to ACPI related > >> Kconfig symbols. Adding him to CC... > > > > So the problem is/was that what happens if you are in a middle of BIOS > > AML code touching the opregion and someone unloads the opregion handler? > > If you can quarantee nothing bad happens, then I'm fine with the patch :) > > > > Rafael to correct me if I got this right. > > The behavior of the operating system is well defined in the ACPI specification. > > Here is what I tested recently: > > ACPI defines _REG method to inform firmware of presence/removal of an operating > region. > > When driver gets loaded, ACPI calls the _REG method with 1 argument. When driver > gets unloaded, ACPI call the _REG method with 0 argument. > > Firmware can use this notification to its advantage to determine when an I2C > related functionality should be accessed or not. > > If firmware doesn't use the _REG method, ACPI defines that AML statements > accessing the operating region are ignored. > > You'll also see a warning from ACPICA saying the OperatingRegion 9 is no longer > accessible and AML code execution failed. > > Also note that someone can always unbind an I2C driver from ACPI even with built-in > module. Yes, that's right. However, the I2C core parts are not removed when you unbind a driver and the imporant piece is i2c_acpi_space_handler() which then stays in the memory even if any of the drivers get removed. After this patch you can also remove the i2c_acpi_space_handler() which might cause issues if there is AML code running at the same time using the opregion. Commit da3c6647ee08 ("I2C/ACPI: Clean up I2C ACPI code and Add CONFIG_I2C_ACPI config") says following: Current there is a race between removing I2C ACPI operation region and ACPI AML code accessing. So make i2c core built-in if CONFIG_I2C_ACPI is set. But I don't remember all the details unfortunately so adding Rafael and Tianyu if they can shed some more light into this ;-)
On Wed, Jan 24, 2018 at 3:29 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > +linux-acpi +Cc: Hans > On 1/24/2018 1:27 AM, Mika Westerberg wrote: >> On Wed, Jan 24, 2018 at 06:56:16AM +0100, Wolfram Sang wrote: >>> On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: >>>> If I2C is built as a module, ACPI_I2C_OPREGION cannot be set >>>> and any ACPI opregion calls targeting I2C fail with no opregion found. >>>> >>>> This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is >>>> built into the kernel or built as a module. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> >>> I recall that we had some discussion until ending up with the current >>> solution. And I finally found it again: >>> >>> http://www.serverphorums.com/read.php?12,1001402 >>> >>> In any case, I surely want Mika's ack on any change to ACPI related >>> Kconfig symbols. Adding him to CC... >> >> So the problem is/was that what happens if you are in a middle of BIOS >> AML code touching the opregion and someone unloads the opregion handler? >> If you can quarantee nothing bad happens, then I'm fine with the patch :) >> > > Rafael to correct me if I got this right. > > The behavior of the operating system is well defined in the ACPI specification. > > Here is what I tested recently: > > ACPI defines _REG method to inform firmware of presence/removal of an operating > region. > > When driver gets loaded, ACPI calls the _REG method with 1 argument. When driver > gets unloaded, ACPI call the _REG method with 0 argument. > > Firmware can use this notification to its advantage to determine when an I2C > related functionality should be accessed or not. > > If firmware doesn't use the _REG method, ACPI defines that AML statements > accessing the operating region are ignored. > > You'll also see a warning from ACPICA saying the OperatingRegion 9 is no longer > accessible and AML code execution failed. > > Also note that someone can always unbind an I2C driver from ACPI even with built-in > module. No, you can't. There are user(s) of that, i.e. PMIC, otherwise, of course, you may do that. > I think we are talking about an orthogonal issue here.
On Wed, Jan 24, 2018 at 8:27 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Wed, Jan 24, 2018 at 06:56:16AM +0100, Wolfram Sang wrote: >> On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: >> > If I2C is built as a module, ACPI_I2C_OPREGION cannot be set >> > and any ACPI opregion calls targeting I2C fail with no opregion found. >> > >> > This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is >> > built into the kernel or built as a module. >> > >> > Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> I recall that we had some discussion until ending up with the current >> solution. And I finally found it again: >> >> http://www.serverphorums.com/read.php?12,1001402 >> >> In any case, I surely want Mika's ack on any change to ACPI related >> Kconfig symbols. Adding him to CC... > > So the problem is/was that what happens if you are in a middle of BIOS > AML code touching the opregion and someone unloads the opregion handler? > If you can quarantee nothing bad happens, then I'm fine with the patch :) I don't think anyone can guarantee this. I would be fine with patch if and only if it has been tested on problematic hardware, such as Intel CherryTrail based tablets / laptops.
On 1/24/2018 9:46 AM, Andy Shevchenko wrote: > On Wed, Jan 24, 2018 at 8:27 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: >> On Wed, Jan 24, 2018 at 06:56:16AM +0100, Wolfram Sang wrote: >>> On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: >>>> If I2C is built as a module, ACPI_I2C_OPREGION cannot be set >>>> and any ACPI opregion calls targeting I2C fail with no opregion found. >>>> >>>> This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is >>>> built into the kernel or built as a module. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> >>> I recall that we had some discussion until ending up with the current >>> solution. And I finally found it again: >>> >>> http://www.serverphorums.com/read.php?12,1001402 >>> >>> In any case, I surely want Mika's ack on any change to ACPI related >>> Kconfig symbols. Adding him to CC... >> >> So the problem is/was that what happens if you are in a middle of BIOS >> AML code touching the opregion and someone unloads the opregion handler? >> If you can quarantee nothing bad happens, then I'm fine with the patch :) > > I don't think anyone can guarantee this. > > I would be fine with patch if and only if it has been tested on > problematic hardware, such as Intel CherryTrail based tablets / > laptops. > Need some details on what the actual issue is/was. One would think that ACPI operating region removal would be serialized with respect to AML execution from 10000 foot view. I'm hoping one of you guys would chime into Mika's message.
Hi, On 24-01-18 15:44, Andy Shevchenko wrote: > On Wed, Jan 24, 2018 at 3:29 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> +linux-acpi > > +Cc: Hans Thank you. >> On 1/24/2018 1:27 AM, Mika Westerberg wrote: >>> On Wed, Jan 24, 2018 at 06:56:16AM +0100, Wolfram Sang wrote: >>>> On Wed, Jan 03, 2018 at 03:02:31PM +0100, Eric Auger wrote: >>>>> If I2C is built as a module, ACPI_I2C_OPREGION cannot be set >>>>> and any ACPI opregion calls targeting I2C fail with no opregion found. >>>>> >>>>> This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is >>>>> built into the kernel or built as a module. >>>>> >>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> >>>> I recall that we had some discussion until ending up with the current >>>> solution. And I finally found it again: >>>> >>>> http://www.serverphorums.com/read.php?12,1001402 >>>> >>>> In any case, I surely want Mika's ack on any change to ACPI related >>>> Kconfig symbols. Adding him to CC... >>> >>> So the problem is/was that what happens if you are in a middle of BIOS >>> AML code touching the opregion and someone unloads the opregion handler? >>> If you can quarantee nothing bad happens, then I'm fine with the patch :) >>> >> >> Rafael to correct me if I got this right. >> >> The behavior of the operating system is well defined in the ACPI specification. >> >> Here is what I tested recently: >> >> ACPI defines _REG method to inform firmware of presence/removal of an operating >> region. >> >> When driver gets loaded, ACPI calls the _REG method with 1 argument. When driver >> gets unloaded, ACPI call the _REG method with 0 argument. >> >> Firmware can use this notification to its advantage to determine when an I2C >> related functionality should be accessed or not. >> >> If firmware doesn't use the _REG method, ACPI defines that AML statements >> accessing the operating region are ignored. >> >> You'll also see a warning from ACPICA saying the OperatingRegion 9 is no longer >> accessible and AML code execution failed. >> There is the ACPI specification, and then there is reality. In reality many DSTDs do not use the typically named AVB? counter-part of _REG to check before accessing OpRegions. Also OpRegion availability vs probe ordering is a nightmare. Lets pretend that all DSTDs are perfect and that some device described in ACPI has a _PS0 method which uses an opregion to turn on some regulator powering the device through i2c, but only if the _REG method for that opregion has been called. So now lets say that the driver for this device loads and tries to bind before the i2c-module has loaded. Before the driver's probe method gets called the driver-core will call _PS0 to power-up the device, which is a nop (*). Then the drivers probe function tries to talk to the device, but fails as the device is not powered, so it returns with -ENODEV. And then later the i2c-module loads, which is to late to get things working, unless the user rmmods and modprobes the driver for the device manually. TL;DR: I have to NAK this, I'm sorry but with the current state of ACPI we must simply have some stuff builtin to help with probe-ordering issues. Now if the ACPI code where ever to honor the _DEP method everywhere instead of only for battery devices this might change, but even then things will still be tricky. Regards, Hans *) Typically while logging errors about unavailable OpRegions, but as said lets pretend DSTDs actually check the flags set by _REG everywhere.
On 1/24/2018 9:59 AM, Hans de Goede wrote: > TL;DR: I have to NAK this, I'm sorry but with the current state of ACPI we > must simply have some stuff builtin to help with probe-ordering issues. Now > if the ACPI code where ever to honor the _DEP method everywhere instead of > only for battery devices this might change, but even then things will still > be tricky. Well, the alternative is even worse. Redhat and most other distros configure I2C as a module. With this setup, I2C OpRegion support does not get compiled. It doesn't even work let alone to have race conditions. I2C OpRegion feature is practically dead for most general users unless you recompile your own kernel. There must be a middle ground somewhere. I had some conversation with Rafael about _DEP support. He is not a big fan :)
Hi, On 24-01-18 16:12, Sinan Kaya wrote: > On 1/24/2018 9:59 AM, Hans de Goede wrote: >> TL;DR: I have to NAK this, I'm sorry but with the current state of ACPI we >> must simply have some stuff builtin to help with probe-ordering issues. Now >> if the ACPI code where ever to honor the _DEP method everywhere instead of >> only for battery devices this might change, but even then things will still >> be tricky. > > Well, the alternative is even worse. No it is not, if I2C gets builtin things work fine, you proposal does not fix anything, it merely gives the illusion of being builtin > Redhat and most other distros configure I2C as a module. Fedora has stopped building I2C as a module a long time ago already and where Fedora goes RHEL typically follows. > With this setup, > I2C OpRegion support does not get compiled. It doesn't even work let alone to have > race conditions. So at least it is consistent then, which makes for a lot easier debugging. > I2C OpRegion feature is practically dead for most general users unless you recompile > your own kernel. Then talk to various distro kernel maintainers and ask them to set CONFIG_I2C=y. > There must be a middle ground somewhere. One thing which comes to mind is to simply not allow building i2c as a module when ACPI is selected, something like this should work I think: config I2C tristate "I2C support" select RT_MUTEXES select IRQ_DOMAIN + # force building I2C in on ACPI systems, for opregion availability + depends on y || !ACPI > I had some conversation with Rafael about _DEP support. He is not a big fan :) Ok. Regards, Hans
On 1/24/2018 10:23 AM, Hans de Goede wrote: >> There must be a middle ground somewhere. > > One thing which comes to mind is to simply not allow building i2c as a module > when ACPI is selected, something like this should work I think: > > config I2C > tristate "I2C support" > select RT_MUTEXES > select IRQ_DOMAIN > + # force building I2C in on ACPI systems, for opregion availability > + depends on y || !ACPI This works for me.
Hi, On 24-01-18 16:37, Sinan Kaya wrote: > On 1/24/2018 10:23 AM, Hans de Goede wrote: >>> There must be a middle ground somewhere. >> >> One thing which comes to mind is to simply not allow building i2c as a module >> when ACPI is selected, something like this should work I think: >> >> config I2C >> tristate "I2C support" >> select RT_MUTEXES >> select IRQ_DOMAIN >> + # force building I2C in on ACPI systems, for opregion availability >> + depends on y || !ACPI > > This works for me. OK, so feel free to turn it into a proper patch and submit it upstream. Regards, Hans
On 1/24/2018 11:09 AM, Hans de Goede wrote: >>>> There must be a middle ground somewhere. >>> >>> One thing which comes to mind is to simply not allow building i2c as a module >>> when ACPI is selected, something like this should work I think: >>> >>> config I2C >>> tristate "I2C support" >>> select RT_MUTEXES >>> select IRQ_DOMAIN >>> + # force building I2C in on ACPI systems, for opregion availability >>> + depends on y || !ACPI >> >> This works for me. > > OK, so feel free to turn it into a proper patch and submit it > upstream. OK. Let me do some build tests.
Hi, On 24/01/18 17:10, Sinan Kaya wrote: > On 1/24/2018 11:09 AM, Hans de Goede wrote: >>>>> There must be a middle ground somewhere. >>>> >>>> One thing which comes to mind is to simply not allow building i2c as a module >>>> when ACPI is selected, something like this should work I think: >>>> >>>> config I2C >>>> tristate "I2C support" >>>> select RT_MUTEXES >>>> select IRQ_DOMAIN >>>> + # force building I2C in on ACPI systems, for opregion availability >>>> + depends on y || !ACPI >>> >>> This works for me. Yes given all the concerns this patch raised, better make sure I2C is built-in along with ACPI. Sorry for the noise. Thanks Eric >> >> OK, so feel free to turn it into a proper patch and submit it >> upstream. > > OK. Let me do some build tests. >
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index efc3354..5951e9d 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -26,7 +26,7 @@ config I2C config ACPI_I2C_OPREGION bool "ACPI I2C Operation region support" - depends on I2C=y && ACPI + depends on I2C && ACPI default y help Say Y here if you want to enable ACPI I2C operation region support.
If I2C is built as a module, ACPI_I2C_OPREGION cannot be set and any ACPI opregion calls targeting I2C fail with no opregion found. This patch allows ACPI_I2C_OPREGION to be enabled both if I2C is built into the kernel or built as a module. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v1 -> v2: - reword the commit message --- drivers/i2c/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)