GED driver is currently set up as a platform driver. On modern operating
systems, most of the drivers are compiled as kernel modules. It is possible
that a GED interrupt event is received and the driver such as GHES/GPIO/I2C
to service it is not available yet. To accommodate this use case, delay
GED driver load to the late init phase.
Signed-off-by: Sinan Kaya <[email protected]>
---
drivers/acpi/evged.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/evged.c b/drivers/acpi/evged.c
index 46f0603..30e638b 100644
--- a/drivers/acpi/evged.c
+++ b/drivers/acpi/evged.c
@@ -151,4 +151,10 @@ static int ged_probe(struct platform_device *pdev)
.acpi_match_table = ACPI_PTR(ged_acpi_ids),
},
};
-builtin_platform_driver(ged_driver);
+
+static __init int ged_init(void)
+{
+ return platform_driver_register(&ged_driver);
+}
+
+late_initcall(ged_init);
--
1.9.1
On Sat, Apr 22, 2017 at 12:28 AM, Sinan Kaya <[email protected]> wrote:
> GED driver is currently set up as a platform driver. On modern operating
> systems, most of the drivers are compiled as kernel modules. It is possible
> that a GED interrupt event is received and the driver such as GHES/GPIO/I2C
> to service it is not available yet. To accommodate this use case, delay
> GED driver load to the late init phase.
>
> Signed-off-by: Sinan Kaya <[email protected]>
> ---
> drivers/acpi/evged.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/evged.c b/drivers/acpi/evged.c
> index 46f0603..30e638b 100644
> --- a/drivers/acpi/evged.c
> +++ b/drivers/acpi/evged.c
> @@ -151,4 +151,10 @@ static int ged_probe(struct platform_device *pdev)
> .acpi_match_table = ACPI_PTR(ged_acpi_ids),
> },
> };
> -builtin_platform_driver(ged_driver);
> +
> +static __init int ged_init(void)
> +{
> + return platform_driver_register(&ged_driver);
> +}
> +
> +late_initcall(ged_init);
Does this fix the problem?
What about if the module in question is loaded after running late_initcalls?
Thanks,
Rafael
On 4/21/2017 6:43 PM, Rafael J. Wysocki wrote:
>> +late_initcall(ged_init);
> Does this fix the problem?
>
> What about if the module in question is loaded after running late_initcalls?
This fixed the issue for me where I had dependencies for QUP I2C driver and GHES
drivers. Both of them are modules and get probed via normal module execution path.
However, I'm open to improvements. Do you have a better suggestion? I can try
to add some _DEP stuff if it is present, but I remember Linux doesn't like _DEP
stuff too much.
--
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.
On 4/21/2017 6:48 PM, Sinan Kaya wrote:
> On 4/21/2017 6:43 PM, Rafael J. Wysocki wrote:
>>> +late_initcall(ged_init);
>> Does this fix the problem?
>>
>> What about if the module in question is loaded after running late_initcalls?
>
> This fixed the issue for me where I had dependencies for QUP I2C driver and GHES
> drivers. Both of them are modules and get probed via normal module execution path.
>
> However, I'm open to improvements. Do you have a better suggestion? I can try
> to add some _DEP stuff if it is present, but I remember Linux doesn't like _DEP
> stuff too much.
>
I forgot to mention that GED driver is a built-in module.
--
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.
On 4/21/2017 4:28 PM, Sinan Kaya wrote:
> GED driver is currently set up as a platform driver. On modern operating
> systems, most of the drivers are compiled as kernel modules. It is possible
> that a GED interrupt event is received and the driver such as GHES/GPIO/I2C
> to service it is not available yet. To accommodate this use case, delay
> GED driver load to the late init phase.
>
> Signed-off-by: Sinan Kaya <[email protected]>
Tested-by: Tyler Baicar <[email protected]>
This resolves the GHES issue described here:
https://lkml.org/lkml/2017/3/28/1083
Thanks,
Tyler
> ---
> drivers/acpi/evged.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/evged.c b/drivers/acpi/evged.c
> index 46f0603..30e638b 100644
> --- a/drivers/acpi/evged.c
> +++ b/drivers/acpi/evged.c
> @@ -151,4 +151,10 @@ static int ged_probe(struct platform_device *pdev)
> .acpi_match_table = ACPI_PTR(ged_acpi_ids),
> },
> };
> -builtin_platform_driver(ged_driver);
> +
> +static __init int ged_init(void)
> +{
> + return platform_driver_register(&ged_driver);
> +}
> +
> +late_initcall(ged_init);
--
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.
On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <[email protected]> wrote:
> On 4/21/2017 6:43 PM, Rafael J. Wysocki wrote:
>>> +late_initcall(ged_init);
>> Does this fix the problem?
>>
>> What about if the module in question is loaded after running late_initcalls?
>
> This fixed the issue for me where I had dependencies for QUP I2C driver and GHES
> drivers. Both of them are modules and get probed via normal module execution path.
>
> However, I'm open to improvements. Do you have a better suggestion? I can try
> to add some _DEP stuff if it is present, but I remember Linux doesn't like _DEP
> stuff too much.
My point is that nothing guarantees a specific ordering or timing of
module loading in general, so moving stuff to different initcall
levels does not really help 100% of the time.
Thanks,
Rafael
Hi Rafael,
On 4/24/2017 7:01 PM, Rafael J. Wysocki wrote:
> On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <[email protected]> wrote:
>> On 4/21/2017 6:43 PM, Rafael J. Wysocki wrote:
>>>> +late_initcall(ged_init);
>>> Does this fix the problem?
>>>
>>> What about if the module in question is loaded after running late_initcalls?
>>
>> This fixed the issue for me where I had dependencies for QUP I2C driver and GHES
>> drivers. Both of them are modules and get probed via normal module execution path.
>>
>> However, I'm open to improvements. Do you have a better suggestion? I can try
>> to add some _DEP stuff if it is present, but I remember Linux doesn't like _DEP
>> stuff too much.
>
> My point is that nothing guarantees a specific ordering or timing of
> module loading in general, so moving stuff to different initcall
> levels does not really help 100% of the time.
>
I was thinking about this today. I agree that this is not a complete solution.
I'm interested in drivers that are either built-in or present in the initramfs.
Drivers that participate in GED work are considered essential drivers. I expect
these essential drivers to be present in early boot phase.
I can certainly improve the commit message.
As long as the drivers are built-in or available in initramfs, I expect this to work.
I want to focus on this use case.
static char *initcall_level_names[] __initdata = {
"early",
"core",
"postcore",
"arch",
"subsys",
"fs",
"device",
"late",
};
static void __init do_initcall_level(int level)
{
...
for (fn = initcall_levels[level]; fn < initcall_levels[level+1]; fn++)
do_one_initcall(*fn);
}
Given these constraints, doesn't this guarantee the order of initialization for built-in and
initramfs modules?
Of course, this won't also play nice with another driver module that requires late_init.
Maybe, this is 1% of the case.
If the driver gets pulled in from the rootfs via modules.conf, then this will definitely
not work as you indicated.
My proposal is to explore the presence of _DEP to reach to %100. Here is an example
GED OBJECT
{
Name(_DEP, "Some other object")
}
I see that ACPI core checks the presence of _DEP value in acpi_device_dep_initialize()
and it won't load the GED driver until dependencies are met if I got it right.
acpi_walk_dep_device_list() gets called from external drivers that need to unblock
the dependent object. acpi_gpiochip_add() seems to take care of this for GPIO.
i2c_acpi_install_space_handler() seems to take care of this for I2C.
We can potentially add acpi_walk_dep_device_list() to GHES driver for completeness.
Then, all FW needs to do is set up a dependency from GED object to its required objects.
Please let me know if I'm missing something.
> Thanks,
> Rafael
> --
> 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
>
Sinan
--
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.
> My point is that nothing guarantees a specific ordering or timing of
> module loading in general, so moving stuff to different initcall
> levels does not really help 100% of the time.
>
Are you talking about init vs. probe in general?
Sorry, I'm trying to decode what you mean to see if there is some other behavior
that I'm not aware of in ACPI.
--
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.
On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <[email protected]> wrote:
> On 4/21/2017 6:43 PM, Rafael J. Wysocki wrote:
> > +late_initcall(ged_init);
> > Does this fix the problem?
> >
> > What about if the module in question is loaded after running
> > late_initcalls?
>
> This fixed the issue for me where I had dependencies for QUP I2C driver
> and GHES drivers. Both of them are modules and get probed via normal
> module execution path.
>
> However, I'm open to improvements. Do you have a better suggestion?
> I can try to add some _DEP stuff if it is present, but I remember Linux
> doesn't like _DEP stuff too much.
Would it be possible to solve this by just returning -EPROBE_DEFER from the
->probe hook if the devices you depend on are not bound yet?
Alternatively, would it be possible to solve it with a struct device_link?
Thanks,
Lukas
On Tue, Apr 25, 2017 at 3:43 AM, Sinan Kaya <[email protected]> wrote:
>
>> My point is that nothing guarantees a specific ordering or timing of
>> module loading in general, so moving stuff to different initcall
>> levels does not really help 100% of the time.
>>
>
> Are you talking about init vs. probe in general?
Yes.
Generally speaking, if the initialization of built-in code depends on
a loadable module to be present, it has to explicitly wait for that
module to advertise itself, this way or another, or it has to poll.
> Sorry, I'm trying to decode what you mean to see if there is some other behavior
> that I'm not aware of in ACPI.
Sorry for being unclear.
Thanks,
Rafael
On 4/25/2017 3:01 AM, Lukas Wunner wrote:
> On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <[email protected]> wrote:
>> On 4/21/2017 6:43 PM, Rafael J. Wysocki wrote:
>>> +late_initcall(ged_init);
>>> Does this fix the problem?
>>>
>>> What about if the module in question is loaded after running
>>> late_initcalls?
>>
>> This fixed the issue for me where I had dependencies for QUP I2C driver
>> and GHES drivers. Both of them are modules and get probed via normal
>> module execution path.
>>
>> However, I'm open to improvements. Do you have a better suggestion?
>> I can try to add some _DEP stuff if it is present, but I remember Linux
>> doesn't like _DEP stuff too much.
>
> Would it be possible to solve this by just returning -EPROBE_DEFER from the
> ->probe hook if the devices you depend on are not bound yet?
>
I'm not sure.
> Alternatively, would it be possible to solve it with a struct device_link?
I wasn't aware of device_link concept. This is something that I will keep in
my mind when I'm dealing with producer/consumer problems with known device
driver instances. It looked very useful.
Here is how the overall relationship between drivers.
| GED | <---> | Platform specific ACPI AML | <----> Vendor GPIO
<----> Vendor I2C
<----> ACPI GHES
<----> Some other driver
The problem with Generic Event Device (GED) is that it produces event
notification facility to the platform specific AML code and GED doesn't
have any idea about the consumers of these interrupts or what platform AML
does.
GED only sees the interrupts that it needs to register and
knows the ASL code it needs to execute when that interrupt happens.
It is possible for AML code not to use any of these drivers or require
some arbitrary driver as well as vendor specific drivers. It is totally
up to the firmware to decide what to do with this event.
My proposal was to require platform AML code to indicate the dependencies
between GED and drivers on the right side of the picture via _DEP as this
cannot be done via normal kernel mechanisms.
This approach might work in general. However, it also has its own caveats.
All of these drivers on the right side are unrelated to each other. Some
operating system can implement a subset of these drivers.
If I include the dependencies, GED will never load for partial driver situations.
This is also a deal breaker.
Why would you break some other feature if your OS doesn't support RAS as an
example?
Given all these lose bindings and no driver association, where do we go
from here?
I consider GED as a light version of Embedded controller (EC) implementation.
How is this problem solved for EC as it has the same problem?
>
> Thanks,
>
> Lukas
>
--
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.
On 4/25/2017 7:03 AM, Rafael J. Wysocki wrote:
>> Are you talking about init vs. probe in general?
> Yes.
>
> Generally speaking, if the initialization of built-in code depends on
> a loadable module to be present, it has to explicitly wait for that
> module to advertise itself, this way or another, or it has to poll.
>
>> Sorry, I'm trying to decode what you mean to see if there is some other behavior
>> that I'm not aware of in ACPI.
> Sorry for being unclear.
Just replied to Lukas. I think I have a question to you about the embedded
controller implementation. Can you check that out?
--
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.
On 4/25/2017 12:24 PM, Sinan Kaya wrote:
> On 4/25/2017 3:01 AM, Lukas Wunner wrote:
>> On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <[email protected]> wrote:
>>> On 4/21/2017 6:43 PM, Rafael J. Wysocki wrote:
>>>> +late_initcall(ged_init);
>>>> Does this fix the problem?
>>>>
>>>> What about if the module in question is loaded after running
>>>> late_initcalls?
>>>
>>> This fixed the issue for me where I had dependencies for QUP I2C driver
>>> and GHES drivers. Both of them are modules and get probed via normal
>>> module execution path.
>>>
>>> However, I'm open to improvements. Do you have a better suggestion?
>>> I can try to add some _DEP stuff if it is present, but I remember Linux
>>> doesn't like _DEP stuff too much.
>>
>> Would it be possible to solve this by just returning -EPROBE_DEFER from the
>> ->probe hook if the devices you depend on are not bound yet?
>>
>
> I'm not sure.
>
>> Alternatively, would it be possible to solve it with a struct device_link?
>
> I wasn't aware of device_link concept. This is something that I will keep in
> my mind when I'm dealing with producer/consumer problems with known device
> driver instances. It looked very useful.
>
> Here is how the overall relationship between drivers.
>
> | GED | <---> | Platform specific ACPI AML | <----> Vendor GPIO
> <----> Vendor I2C
> <----> ACPI GHES
> <----> Some other driver
>
> The problem with Generic Event Device (GED) is that it produces event
> notification facility to the platform specific AML code and GED doesn't
> have any idea about the consumers of these interrupts or what platform AML
> does.
>
> GED only sees the interrupts that it needs to register and
> knows the ASL code it needs to execute when that interrupt happens.
>
> It is possible for AML code not to use any of these drivers or require
> some arbitrary driver as well as vendor specific drivers. It is totally
> up to the firmware to decide what to do with this event.
>
> My proposal was to require platform AML code to indicate the dependencies
> between GED and drivers on the right side of the picture via _DEP as this
> cannot be done via normal kernel mechanisms.
>
> This approach might work in general. However, it also has its own caveats.
>
> All of these drivers on the right side are unrelated to each other. Some
> operating system can implement a subset of these drivers.
>
> If I include the dependencies, GED will never load for partial driver situations.
> This is also a deal breaker.
>
> Why would you break some other feature if your OS doesn't support RAS as an
> example?
>
> Given all these lose bindings and no driver association, where do we go
> from here?
>
> I consider GED as a light version of Embedded controller (EC) implementation.
>
> How is this problem solved for EC as it has the same problem?
>
This recommendation came from Timur. I wanted to see how everybody feels about it.
When GED driver makes an AML call and the driver on the right side of the picture
is not present, GED driver gets an ACPI error return code.
GED driver can potentially reschedule the AML call to be retried in 30 seconds.
Repeat this 5 times. If nobody handles the event, disable the interrupt.
Let me know what you think.
--
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.