[v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module

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
Related show

Commit Message

Auger Eric Jan. 3, 2018, 2:02 p.m.
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(-)

Comments

Sinan Kaya Jan. 8, 2018, 4:22 p.m. | #1
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>
Wolfram Sang Jan. 24, 2018, 5:56 a.m. | #2
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...
Mika Westerberg Jan. 24, 2018, 6:27 a.m. | #3
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 :)
Sinan Kaya Jan. 24, 2018, 1:29 p.m. | #4
+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.
Mika Westerberg Jan. 24, 2018, 2:24 p.m. | #5
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 ;-)
Andy Shevchenko Jan. 24, 2018, 2:44 p.m. | #6
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.
Andy Shevchenko Jan. 24, 2018, 2:46 p.m. | #7
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.
Sinan Kaya Jan. 24, 2018, 2:49 p.m. | #8
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.
Hans de Goede Jan. 24, 2018, 2:59 p.m. | #9
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.
Sinan Kaya Jan. 24, 2018, 3:12 p.m. | #10
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 :)
Hans de Goede Jan. 24, 2018, 3:23 p.m. | #11
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
Sinan Kaya Jan. 24, 2018, 3:37 p.m. | #12
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.
Hans de Goede Jan. 24, 2018, 4:09 p.m. | #13
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
Sinan Kaya Jan. 24, 2018, 4:10 p.m. | #14
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.
Auger Eric Jan. 24, 2018, 4:56 p.m. | #15
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.
>

Patch

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.