ACPI: Force I2C to be selected as a built-in module

PATCH In review Sinan Kaya 5 participants 11 messages Jan 25, 2018 – Feb 8, 2018

Subsystems (2)

ACPI I2C subsystem host drivers

lore

Share & tools

lore

Sinan Kaya

author

2018-01-25 15:43 UTC #0 lore raw

PATCH v3 1/1 ACPI: Force I2C to be selected as a built-in module

From: Auger Eric <[email protected]>

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.

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

This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.

Signed-off-by Eric Auger <[email protected]>

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

drivers/acpi/Kconfig +2 -0

index 4650539..5b48098 100644
@@ -9,6 +9,8 @@ menuconfig ACPI
 	depends on IA64 || X86 || ARM64
 	depends on PCI
 	select PNP
+	# force building I2C in on ACPI systems, for opregion availability
+	imply I2C
 	default y if (IA64 || X86)
 	help
 	  Advanced Configuration and Power Interface (ACPI) support for

Signature · 1 line

1.9.1

Rafael J. Wysocki

maintainer

2018-01-25 16:25 UTC #1 lore raw

Quoted 26 lines · Sinan Kaya

From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644

drivers/acpi/Kconfig menuconfig ACPI

Quoted 9 lines

drivers/acpi/Kconfig +2 -1

@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--

I'm not sure how much this helps.

I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAICS.

OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?

Thanks,
Rafael

Sinan Kaya

author

2018-01-25 16:32 UTC #2 lore raw

Quoted 39 lines · Rafael J. Wysocki

Quoted 26 lines · Sinan Kaya
From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644
drivers/acpi/Kconfig menuconfig ACPI
Quoted 9 lines
drivers/acpi/Kconfig +2 -1
@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--
I'm not sure how much this helps.

Quoted 4 lines

I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAIC> OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?

I was trying to find a good balance by choosing imply instead of select.

If an OS chooses to include an I2C driver as a module (most distros do), core-i2c functionality becomes a built-in module with ACPI.

If an OS doesn't choose any I2C drivers either built-in/module, then I2C core functionality wouldn't be selected; right?

Quoted 4 lines

Thanks,
Rafael

Signature · 3 lines

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.

Rafael J. Wysocki

maintainer

2018-01-25 16:47 UTC #3 lore raw

Quoted 45 lines · Sinan Kaya

Quoted 39 lines · Rafael J. Wysocki
Quoted 26 lines · Sinan Kaya
From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644
drivers/acpi/Kconfig menuconfig ACPI
Quoted 9 lines
drivers/acpi/Kconfig +2 -1
@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--
I'm not sure how much this helps.
Quoted 4 lines
I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAIC> OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?

Quoted 4 lines

I was trying to find a good balance by choosing imply instead of select.
If an OS chooses to include an I2C driver as a module (most distros do), core-i2c functionality becomes a built-in module with ACPI.

So what would be wrong with allowing ACPI_I2C_OPREGION to be set if I2C is a module?

Sinan Kaya

author

2018-01-25 16:53 UTC #4 lore raw

Quoted 46 lines · Rafael J. Wysocki

Quoted 45 lines · Sinan Kaya
Quoted 43 lines · Rafael J. Wysocki
Quoted 26 lines · Sinan Kaya
From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644
drivers/acpi/Kconfig menuconfig ACPI
Quoted 9 lines
drivers/acpi/Kconfig +2 -1
@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--
I'm not sure how much this helps.
I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAIC> OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?

Quoted 7 lines

Quoted 4 lines
I was trying to find a good balance by choosing imply instead of select.
If an OS chooses to include an I2C driver as a module (most distros do), core-i2c functionality becomes a built-in module with ACPI.
So what would be wrong with allowing ACPI_I2C_OPREGION to be set if I2C is a module?

That was the original patch. Folks are saying that it breaks some systems.

Conversation is here:

https://patchwork.kernel.org/patch/10142425/

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

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.

Rafael J. Wysocki

maintainer

2018-01-25 16:57 UTC #5 lore raw

Quoted 52 lines · Sinan Kaya

Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Sinan Kaya
Quoted 39 lines · Rafael J. Wysocki
Quoted 26 lines · Sinan Kaya
From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644
drivers/acpi/Kconfig menuconfig ACPI
Quoted 9 lines
drivers/acpi/Kconfig +2 -1
@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--
I'm not sure how much this helps.
Quoted 4 lines
I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAIC> OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?
I was trying to find a good balance by choosing imply instead of select.
If an OS chooses to include an I2C driver as a module (most distros do), core-i2c functionality becomes a built-in module with ACPI.

Quoted 8 lines

Quoted 2 lines
So what would be wrong with allowing ACPI_I2C_OPREGION to be set if I2C is a module?
That was the original patch. Folks are saying that it breaks some systems.
Conversation is here:
https://patchwork.kernel.org/patch/10142425/

Couldn't that be addressed by preventing the I2C module from being unloaded in the relevant cases?

Sinan Kaya

author

2018-01-25 17:07 UTC #6 lore raw

Quoted 8 lines · Rafael J. Wysocki

Quoted 5 lines
That was the original patch. Folks are saying that it breaks some systems.
Conversation is here:
https://patchwork.kernel.org/patch/10142425/
Couldn't that be addressed by preventing the I2C module from being unloaded in the relevant cases?

Maybe, I'll let Andy And Hans speak for themselves. It is not my code.

I listed the spec behavior and expectation from the OS in the link above. Apparently, Linux isn't playing nice.

Hans is also raising a concern about driver load order that is not being handled well between the I2C driver, its sub modules like PMIC and the ACPI support.

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

I'd think that driver would return -EPROBE_DEFER in order to wait until its dependencies come in place. Maybe, driver was not designed for this.

Signature · 3 lines

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.

Hans de Goede

2018-01-25 17:11 UTC #7 lore raw

Hi,

Quoted 27 lines · Sinan Kaya

Quoted 8 lines · Rafael J. Wysocki
Quoted 5 lines
That was the original patch. Folks are saying that it breaks some systems.
Conversation is here:
https://patchwork.kernel.org/patch/10142425/
Couldn't that be addressed by preventing the I2C module from being unloaded in the relevant cases?
Maybe, I'll let Andy And Hans speak for themselves. It is not my code.
I listed the spec behavior and expectation from the OS in the link above. Apparently, Linux isn't playing nice.
Hans is also raising a concern about driver load order that is not being handled well between the I2C driver, its sub modules like PMIC and the ACPI support.
"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."

Quoted 2 lines

I'd think that driver would return -EPROBE_DEFER in order to wait until its dependencies come in place. Maybe, driver was not designed for this.

The driver is not aware of any power-management done by ACPI, so the driver cannot return -EPROBE_DEFER.

Regards,

Hans

Randy Dunlap

2018-01-25 17:36 UTC #8 lore raw

In reply to Rafael J. Wysocki

Quoted 39 lines · Rafael J. Wysocki

Quoted 26 lines · Sinan Kaya
From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644
drivers/acpi/Kconfig menuconfig ACPI
Quoted 9 lines
drivers/acpi/Kconfig +2 -1
@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--
I'm not sure how much this helps.

Quoted 5 lines

I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAICS.
OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?

Definitely not.

Signature · 1 line

~Randy

Sinan Kaya

author

2018-02-06 14:25 UTC #9 lore raw

Hi Rafael,

Quoted 40 lines · Randy Dunlap

Quoted 39 lines · Rafael J. Wysocki
Quoted 26 lines · Sinan Kaya
From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644
drivers/acpi/Kconfig menuconfig ACPI
Quoted 9 lines
drivers/acpi/Kconfig +2 -1
@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--
I'm not sure how much this helps.

Quoted 8 lines

Quoted 5 lines
I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAICS.
OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?
Definitely not.

Where do we stand on this? Do you have a better suggestion?

Sinan

Signature · 3 lines

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.

Rafael J. Wysocki

2018-02-08 09:52 UTC #10 lore raw

Quoted 46 lines · CET Sinan Kaya

Hi Rafael,
Quoted 43 lines · Randy Dunlap
Quoted 42 lines · Rafael J. Wysocki
Quoted 26 lines · Sinan Kaya
From: Auger Eric <[email protected]>
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.
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."
This patch forces core I2C support to be compiled as a built-in module if ACPI is selected as code is not ready for dynamic module removal.
Signed-off-by Eric Auger <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/acpi/Kconfig +0 -0
index 4650539..5b48098 100644
drivers/acpi/Kconfig menuconfig ACPI
Quoted 9 lines
drivers/acpi/Kconfig +2 -1
@@ -9,6 +9,8 @@ menuconfig ACPI
        depends on IA64 || X86 || ARM64
        depends on PCI
        select PNP
+       # force building I2C in on ACPI systems, for opregion availability
+       imply I2C
        default y if (IA64 || X86)
        help
          Advanced Configuration and Power Interface (ACPI) support for
--
I'm not sure how much this helps.
I2C opregions will only work if the requisite I2C controller driver is present anyway and this change doesn't guarantee that AFAICS.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
OTOH, there are systems using ACPI without I2C opregions, so are we really better off by forcing everybody using ACPI to also build I2C?
Definitely not.
Where do we stand on this? Do you have a better suggestion?

Not at the moment, but I also don't think that this approach is valid.

Thanks,
Rafael