| Message ID | 1516895011-17588-1-git-send-email-okaya@codeaurora.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v3] ACPI: Force I2C to be selected as a built-in module | expand |
On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > From: Auger Eric <eric.auger@redhat.com> > > 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 <eric.auger@redhat.com> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/acpi/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index 4650539..5b48098 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -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
On 1/25/2018 11:25 AM, Rafael J. Wysocki wrote: > On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> From: Auger Eric <eric.auger@redhat.com> >> >> 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 <eric.auger@redhat.com> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> --- >> drivers/acpi/Kconfig | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index 4650539..5b48098 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -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? 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? > > Thanks, > Rafael >
On Thu, Jan 25, 2018 at 5:32 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 1/25/2018 11:25 AM, Rafael J. Wysocki wrote: >> On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>> From: Auger Eric <eric.auger@redhat.com> >>> >>> 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 <eric.auger@redhat.com> >>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >>> --- >>> drivers/acpi/Kconfig | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >>> index 4650539..5b48098 100644 >>> --- a/drivers/acpi/Kconfig >>> +++ b/drivers/acpi/Kconfig >>> @@ -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? > > 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?
On 1/25/2018 11:47 AM, Rafael J. Wysocki wrote: > On Thu, Jan 25, 2018 at 5:32 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> On 1/25/2018 11:25 AM, Rafael J. Wysocki wrote: >>> On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>>> From: Auger Eric <eric.auger@redhat.com> >>>> >>>> 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 <eric.auger@redhat.com> >>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >>>> --- >>>> drivers/acpi/Kconfig | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >>>> index 4650539..5b48098 100644 >>>> --- a/drivers/acpi/Kconfig >>>> +++ b/drivers/acpi/Kconfig >>>> @@ -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? >> >> 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/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thu, Jan 25, 2018 at 5:53 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 1/25/2018 11:47 AM, Rafael J. Wysocki wrote: >> On Thu, Jan 25, 2018 at 5:32 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>> On 1/25/2018 11:25 AM, Rafael J. Wysocki wrote: >>>> On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>>>> From: Auger Eric <eric.auger@redhat.com> >>>>> >>>>> 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 <eric.auger@redhat.com> >>>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >>>>> --- >>>>> drivers/acpi/Kconfig | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >>>>> index 4650539..5b48098 100644 >>>>> --- a/drivers/acpi/Kconfig >>>>> +++ b/drivers/acpi/Kconfig >>>>> @@ -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? >>> >>> 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/ Couldn't that be addressed by preventing the I2C module from being unloaded in the relevant cases?
On 1/25/2018 11:57 AM, Rafael J. Wysocki wrote: >> 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.
Hi, On 25-01-18 18:07, Sinan Kaya wrote: > On 1/25/2018 11:57 AM, Rafael J. Wysocki wrote: >>> 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. The driver is not aware of any power-management done by ACPI, so the driver cannot return -EPROBE_DEFER. Regards, Hans
On 01/25/2018 08:25 AM, Rafael J. Wysocki wrote: > On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> From: Auger Eric <eric.auger@redhat.com> >> >> 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 <eric.auger@redhat.com> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> --- >> drivers/acpi/Kconfig | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index 4650539..5b48098 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -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? Definitely not.
Hi Rafael, On 1/25/2018 12:36 PM, Randy Dunlap wrote: > On 01/25/2018 08:25 AM, Rafael J. Wysocki wrote: >> On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>> From: Auger Eric <eric.auger@redhat.com> >>> >>> 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 <eric.auger@redhat.com> >>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >>> --- >>> drivers/acpi/Kconfig | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >>> index 4650539..5b48098 100644 >>> --- a/drivers/acpi/Kconfig >>> +++ b/drivers/acpi/Kconfig >>> @@ -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? > > Definitely not. > Where do we stand on this? Do you have a better suggestion? Sinan
On Tuesday, February 6, 2018 3:25:58 PM CET Sinan Kaya wrote: > Hi Rafael, > > On 1/25/2018 12:36 PM, Randy Dunlap wrote: > > On 01/25/2018 08:25 AM, Rafael J. Wysocki wrote: > >> On Thu, Jan 25, 2018 at 4:43 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > >>> From: Auger Eric <eric.auger@redhat.com> > >>> > >>> 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 <eric.auger@redhat.com> > >>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > >>> --- > >>> drivers/acpi/Kconfig | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > >>> index 4650539..5b48098 100644 > >>> --- a/drivers/acpi/Kconfig > >>> +++ b/drivers/acpi/Kconfig > >>> @@ -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? > > > > 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
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 4650539..5b48098 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -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