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

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

[Sinan Kaya]

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

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Mika Westerberg]

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 ;-)

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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.

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

[Sinan Kaya]

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

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Hans de Goede]

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

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

[Sinan Kaya]

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.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Hans de Goede]

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

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

[Sinan Kaya]

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.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Auger Eric]

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