linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
@ 2018-01-03 14:02 Eric Auger
  2018-01-08 16:22 ` Sinan Kaya
  2018-01-24  5:56 ` Wolfram Sang
  0 siblings, 2 replies; 16+ messages in thread
From: Eric Auger @ 2018-01-03 14:02 UTC (permalink / raw)
  To: eric.auger.pro, eric.auger, wsa, linux-i2c, linux-kernel, okaya

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

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-03 14:02 [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module Eric Auger
@ 2018-01-08 16:22 ` Sinan Kaya
  2018-01-24  5:56 ` Wolfram Sang
  1 sibling, 0 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-01-08 16:22 UTC (permalink / raw)
  To: Eric Auger, eric.auger.pro, wsa, linux-i2c, linux-kernel

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>

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-03 14:02 [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module Eric Auger
  2018-01-08 16:22 ` Sinan Kaya
@ 2018-01-24  5:56 ` Wolfram Sang
  2018-01-24  6:27   ` Mika Westerberg
  1 sibling, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2018-01-24  5:56 UTC (permalink / raw)
  To: Eric Auger, Mika Westerberg
  Cc: eric.auger.pro, linux-i2c, linux-kernel, okaya

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24  5:56 ` Wolfram Sang
@ 2018-01-24  6:27   ` Mika Westerberg
  2018-01-24 13:29     ` Sinan Kaya
  2018-01-24 14:46     ` Andy Shevchenko
  0 siblings, 2 replies; 16+ messages in thread
From: Mika Westerberg @ 2018-01-24  6:27 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: Eric Auger, eric.auger.pro, linux-i2c, linux-kernel, okaya

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24  6:27   ` Mika Westerberg
@ 2018-01-24 13:29     ` Sinan Kaya
  2018-01-24 14:24       ` Mika Westerberg
  2018-01-24 14:44       ` Andy Shevchenko
  2018-01-24 14:46     ` Andy Shevchenko
  1 sibling, 2 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-01-24 13:29 UTC (permalink / raw)
  To: Mika Westerberg, Wolfram Sang
  Cc: Eric Auger, eric.auger.pro, linux-i2c, linux-kernel, linux-acpi

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 13:29     ` Sinan Kaya
@ 2018-01-24 14:24       ` Mika Westerberg
  2018-01-24 14:44       ` Andy Shevchenko
  1 sibling, 0 replies; 16+ messages in thread
From: Mika Westerberg @ 2018-01-24 14:24 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Wolfram Sang, Eric Auger, eric.auger.pro, linux-i2c,
	linux-kernel, linux-acpi, Rafael J. Wysocki, Lan Tianyu

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 13:29     ` Sinan Kaya
  2018-01-24 14:24       ` Mika Westerberg
@ 2018-01-24 14:44       ` Andy Shevchenko
  2018-01-24 14:59         ` Hans de Goede
  1 sibling, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-01-24 14:44 UTC (permalink / raw)
  To: Sinan Kaya, Hans de Goede
  Cc: Mika Westerberg, Wolfram Sang, Eric Auger, eric.auger.pro,
	linux-i2c, Linux Kernel Mailing List, linux-acpi

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24  6:27   ` Mika Westerberg
  2018-01-24 13:29     ` Sinan Kaya
@ 2018-01-24 14:46     ` Andy Shevchenko
  2018-01-24 14:49       ` Sinan Kaya
  1 sibling, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-01-24 14:46 UTC (permalink / raw)
  To: Mika Westerberg, Hans de Goede
  Cc: Wolfram Sang, Eric Auger, eric.auger.pro, linux-i2c,
	Linux Kernel Mailing List, Sinan Kaya

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.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 14:46     ` Andy Shevchenko
@ 2018-01-24 14:49       ` Sinan Kaya
  0 siblings, 0 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-01-24 14:49 UTC (permalink / raw)
  To: Andy Shevchenko, Mika Westerberg, Hans de Goede
  Cc: Wolfram Sang, Eric Auger, eric.auger.pro, linux-i2c,
	Linux Kernel Mailing List

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.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 14:44       ` Andy Shevchenko
@ 2018-01-24 14:59         ` Hans de Goede
  2018-01-24 15:12           ` Sinan Kaya
  0 siblings, 1 reply; 16+ messages in thread
From: Hans de Goede @ 2018-01-24 14:59 UTC (permalink / raw)
  To: Andy Shevchenko, Sinan Kaya
  Cc: Mika Westerberg, Wolfram Sang, Eric Auger, eric.auger.pro,
	linux-i2c, Linux Kernel Mailing List, linux-acpi

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 14:59         ` Hans de Goede
@ 2018-01-24 15:12           ` Sinan Kaya
  2018-01-24 15:23             ` Hans de Goede
  0 siblings, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-01-24 15:12 UTC (permalink / raw)
  To: Hans de Goede, Andy Shevchenko
  Cc: Mika Westerberg, Wolfram Sang, Eric Auger, eric.auger.pro,
	linux-i2c, Linux Kernel Mailing List, linux-acpi

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 15:12           ` Sinan Kaya
@ 2018-01-24 15:23             ` Hans de Goede
  2018-01-24 15:37               ` Sinan Kaya
  0 siblings, 1 reply; 16+ messages in thread
From: Hans de Goede @ 2018-01-24 15:23 UTC (permalink / raw)
  To: Sinan Kaya, Andy Shevchenko
  Cc: Mika Westerberg, Wolfram Sang, Eric Auger, eric.auger.pro,
	linux-i2c, Linux Kernel Mailing List, linux-acpi

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 15:23             ` Hans de Goede
@ 2018-01-24 15:37               ` Sinan Kaya
  2018-01-24 16:09                 ` Hans de Goede
  0 siblings, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-01-24 15:37 UTC (permalink / raw)
  To: Hans de Goede, Andy Shevchenko
  Cc: Mika Westerberg, Wolfram Sang, Eric Auger, eric.auger.pro,
	linux-i2c, Linux Kernel Mailing List, linux-acpi

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 15:37               ` Sinan Kaya
@ 2018-01-24 16:09                 ` Hans de Goede
  2018-01-24 16:10                   ` Sinan Kaya
  0 siblings, 1 reply; 16+ messages in thread
From: Hans de Goede @ 2018-01-24 16:09 UTC (permalink / raw)
  To: Sinan Kaya, Andy Shevchenko
  Cc: Mika Westerberg, Wolfram Sang, Eric Auger, eric.auger.pro,
	linux-i2c, Linux Kernel Mailing List, linux-acpi

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 16:09                 ` Hans de Goede
@ 2018-01-24 16:10                   ` Sinan Kaya
  2018-01-24 16:56                     ` Auger Eric
  0 siblings, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-01-24 16:10 UTC (permalink / raw)
  To: Hans de Goede, Andy Shevchenko
  Cc: Mika Westerberg, Wolfram Sang, Eric Auger, eric.auger.pro,
	linux-i2c, Linux Kernel Mailing List, linux-acpi

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module
  2018-01-24 16:10                   ` Sinan Kaya
@ 2018-01-24 16:56                     ` Auger Eric
  0 siblings, 0 replies; 16+ messages in thread
From: Auger Eric @ 2018-01-24 16:56 UTC (permalink / raw)
  To: Sinan Kaya, Hans de Goede, Andy Shevchenko
  Cc: Mika Westerberg, Wolfram Sang, eric.auger.pro, linux-i2c,
	Linux Kernel Mailing List, linux-acpi

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-01-24 16:56 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-03 14:02 [PATCH v2] i2c: Allow ACPI_I2C_OPREGION if I2C is built as a module Eric Auger
2018-01-08 16:22 ` Sinan Kaya
2018-01-24  5:56 ` Wolfram Sang
2018-01-24  6:27   ` Mika Westerberg
2018-01-24 13:29     ` Sinan Kaya
2018-01-24 14:24       ` Mika Westerberg
2018-01-24 14:44       ` Andy Shevchenko
2018-01-24 14:59         ` Hans de Goede
2018-01-24 15:12           ` Sinan Kaya
2018-01-24 15:23             ` Hans de Goede
2018-01-24 15:37               ` Sinan Kaya
2018-01-24 16:09                 ` Hans de Goede
2018-01-24 16:10                   ` Sinan Kaya
2018-01-24 16:56                     ` Auger Eric
2018-01-24 14:46     ` Andy Shevchenko
2018-01-24 14:49       ` Sinan Kaya

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).