[PATCH] ACPI / GED: use late init to allow other drivers init

[PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

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 <okaya@codeaurora.org>
---
 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

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Rafael J. Wysocki]

On Sat, Apr 22, 2017 at 12:28 AM, Sinan Kaya <okaya@codeaurora.org> 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 <okaya@codeaurora.org>
> ---
>  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

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

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.

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

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.

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Rafael J. Wysocki]

On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <okaya@codeaurora.org> 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

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

Hi Rafael,

On 4/24/2017 7:01 PM, Rafael J. Wysocki wrote:
> On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <okaya@codeaurora.org> 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 majordomo@vger.kernel.org
> 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.

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Lukas Wunner]

On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <okaya@codeaurora.org> 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

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

On 4/25/2017 3:01 AM, Lukas Wunner wrote:
> On Sat, Apr 22, 2017 at 12:48 AM, Sinan Kaya <okaya@codeaurora.org> 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.

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

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 <okaya@codeaurora.org> 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.

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Rafael J. Wysocki]

On Thursday, April 27, 2017 10:32:07 PM Sinan Kaya wrote:
> 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 <okaya@codeaurora.org> 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. 

This means that _EVT evaluation failed, right?

How does the _EVT in question look like?  What does make it depend on the
other drivers to be present in particular?

Thanks,
Rafael

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

On 5/10/2017 8:58 PM, Rafael J. Wysocki wrote:
>> 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. 
> This means that _EVT evaluation failed, right?
> 
> How does the _EVT in question look like?  What does make it depend on the
> other drivers to be present in particular?

This one was trying to use an I2C Operating Region. The QUP I2C driver was not loaded
yet. Since the I2C namespace handler was not registered, ACPI returned an error.


[    5.061989] ACPI Error: Result stack is empty! State=ffffe021fd9eac00 (20160930/dswstate-99)^M
[    5.061992] ACPI Error: Method parse/execution failed [\_SB.I1C2.ARBT.LCK] (Node ffffe0213c154988), AE_NOT_EXIST (20160930/psparse-543)^M
[    5.061998] ACPI Error: Method parse/execution failed [\_SB.I1C2.PDV0.GETI] (Node ffffe0213c154410), AE_NOT_EXIST (20160930/psparse-543)^M
[    5.062003] ACPI Error: Method parse/execution failed [\_SB.PHP0.GETI] (Node ffffe0213c135118), AE_NOT_EXIST (20160930/psparse-543)^M
[    5.062009] ACPI Error: Method parse/execution failed [\_SB.PHP0.PVNH] (Node ffffe0213c135dc0), AE_NOT_EXIST (20160930/psparse-543)^M
[    5.062013] ACPI Error: Method parse/execution failed [\_SB.PCI0.PEVN] (Node ffffe0213c133910), AE_NOT_EXIST (20160930/psparse-543)^M
[    5.062017] ACPI Error: Method parse/execution failed [\_SB.GED1.PCIM] (Node ffffe0213c121398), AE_NOT_EXIST (20160930/psparse-543)^M
[    5.062022] ACPI Error: Method parse/execution failed [\_SB.GED1._EVT] (Node ffffe0213c1217d0), AE_NOT_EXIST (20160930/psparse-543)^M


-- 
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] ACPI / GED: use late init to allow other drivers init

[Rafael J. Wysocki]

Sorry for the delay.

On Tuesday, April 25, 2017 12:24:19 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 <okaya@codeaurora.org> 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.

Something like _DEP would be needed.

However, _DEP as specified is only about operation region dependencies, which
doesn't seem to be applicable here.

That said, _DEP is used for general dependecies by firmware already, but it
would at least be good to send a proposal for a spec update regarding that
before mandating using _DEP for GED.

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

_DEP doesn't mean a hard dependency AFAICS.  It is about ordering, not about
presence, at least as specified currently.

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

No, it is not.

It is more of a generalization of the GPE/SCI mechanism in order to make it
possible to cover things different from GPIO (which already is covered by
_AEI).

> How is this problem solved for EC as it has the same problem?

It doesn't.  The EC relies on the GPE/SCI mechanism to be there and that is
always present.

Thanks,
Rafael

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

Hi Rafael,

On 5/10/2017 8:46 PM, Rafael J. Wysocki wrote:
>> 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.
> Something like _DEP would be needed.
> 
> However, _DEP as specified is only about operation region dependencies, which
> doesn't seem to be applicable here.
> 
> That said, _DEP is used for general dependecies by firmware already, but it
> would at least be good to send a proposal for a spec update regarding that
> before mandating using _DEP for GED.

OK. I'll reach out to Harb and let's see where the proposal goes. 

> 
>> 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. 
> _DEP doesn't mean a hard dependency AFAICS.  It is about ordering, not about
> presence, at least as specified currently.
> 
>> 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. 
> No, it is not.

Thanks for correction. Let me repeat with the correct terminology this time. 

Don't we have the same problem on GPE/SCI mechanism?

An event that SCI is delivering may not be handled because the handler of the
event is not present during OS boot?

The SCI relationship would be:

| SCI | <--->  | Platform specific ACPI AML (_AEI) | <----> Vendor XYZ driver
                                              	     <----> Vendor I2C
                                                     <----> ACPI GHES

> 
> It is more of a generalization of the GPE/SCI mechanism in order to make it
> possible to cover things different from GPIO (which already is covered by
> _AEI).
> 
>> How is this problem solved for EC as it has the same problem?
> It doesn't.  The EC relies on the GPE/SCI mechanism to be there and that is
> always present.
> 
> Thanks,
> Rafael


-- 
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] ACPI / GED: use late init to allow other drivers init

[Rafael J. Wysocki]

On Thursday, May 11, 2017 09:43:14 AM Sinan Kaya wrote:
> Hi Rafael,
> 
> On 5/10/2017 8:46 PM, Rafael J. Wysocki wrote:
> >> 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.
> > Something like _DEP would be needed.
> > 
> > However, _DEP as specified is only about operation region dependencies, which
> > doesn't seem to be applicable here.
> > 
> > That said, _DEP is used for general dependecies by firmware already, but it
> > would at least be good to send a proposal for a spec update regarding that
> > before mandating using _DEP for GED.
> 
> OK. I'll reach out to Harb and let's see where the proposal goes. 

It looks like this is about operation regions after all, however, so _DEP as is
should be sufficient here.

There is some limited _DEP support in the ACPI layer, but we were missing
a way to represent those dependencies in the driver core.

That can be done through device_link objects now, so we may be able to support
_DEP in a more meaningful way, but the cases when _DEP is used for something
different from operation regions in practice need to be treated with caution.


> > 
> >> 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. 
> > _DEP doesn't mean a hard dependency AFAICS.  It is about ordering, not about
> > presence, at least as specified currently.
> > 
> >> 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. 
> > No, it is not.
> 
> Thanks for correction. Let me repeat with the correct terminology this time. 
> 
> Don't we have the same problem on GPE/SCI mechanism?

Yes, in theory.

> An event that SCI is delivering may not be handled because the handler of the
> event is not present during OS boot?

It would be a problem if something like _Lxx or _Exx referred to an operation
region without a handler, but these usually refer to operation regions in
memory or in the PCI config space and there are default operation region
handlers for those address spaces.

Of course, if they referred to operation regions on an I2C bus, for example,
the problem might very well occur in there too.

> The SCI relationship would be:
> 
> | SCI | <--->  | Platform specific ACPI AML (_AEI) | <----> Vendor XYZ driver
>                                               	     <----> Vendor I2C
>                                                      <----> ACPI GHES
> 

This would not be the SCI AFAICS, because _AEI is specific to GPIO interrupts
and it only says which interrupts should be handled by the ACPI layer.  There
needs to be _EVT for handling them just like in the GED case (or the other
way around, actuallly), so this is just analogous to GED.

Thanks,
Rafael

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

Hi Rafael,

On 5/11/2017 10:52 AM, Rafael J. Wysocki wrote:
>> OK. I'll reach out to Harb and let's see where the proposal goes. 
> It looks like this is about operation regions after all, however, so _DEP as is
> should be sufficient here.
> 
> There is some limited _DEP support in the ACPI layer, but we were missing
> a way to represent those dependencies in the driver core.
> 
> That can be done through device_link objects now, so we may be able to support
> _DEP in a more meaningful way, but the cases when _DEP is used for something
> different from operation regions in practice need to be treated with caution.
> 
> 

_DEP could certainly help here. However, _DEP doesn't answer the loose binding question.
If one driver is missing in one operating system, _GED driver will never load due 
to unsatisfied dependency. This forces us into all or none condition. We have operating
systems that we need to support and do not have vendor I2C or GPIO drivers. That's why,
we are hesitant to place _DEP into ACPI tables.

The problem is that there is no concept of per event dependency. This could have helped
us figure out if such an interrupt should be enabled or not.

Another solution for operating regions is _REG if FW wants to ignore the event during
boot. This is the one we are looking into at this moment for non-critical events. 

late_init proposed in this patch helps for built-in drivers such as GHES where we do
not want to ignore events. Since GHES is not an actual device, this has to be solved
in ACPI.


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.

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Rafael J. Wysocki]

On Mon, May 15, 2017 at 4:36 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Hi Rafael,
>
> On 5/11/2017 10:52 AM, Rafael J. Wysocki wrote:
>>> OK. I'll reach out to Harb and let's see where the proposal goes.
>> It looks like this is about operation regions after all, however, so _DEP as is
>> should be sufficient here.
>>
>> There is some limited _DEP support in the ACPI layer, but we were missing
>> a way to represent those dependencies in the driver core.
>>
>> That can be done through device_link objects now, so we may be able to support
>> _DEP in a more meaningful way, but the cases when _DEP is used for something
>> different from operation regions in practice need to be treated with caution.
>>
>>
>
> _DEP could certainly help here. However, _DEP doesn't answer the loose binding question.
> If one driver is missing in one operating system, _GED driver will never load due
> to unsatisfied dependency. This forces us into all or none condition. We have operating
> systems that we need to support and do not have vendor I2C or GPIO drivers. That's why,
> we are hesitant to place _DEP into ACPI tables.

_DEP as defined in the spec should be fine still.

Quoting directly:

"_DEP evaluates to a package and designates device objects that OSPM
should assign a higher
priority in start ordering due to future operation region accesses.

To increase the likelihood that an SPB operation region handler is
available when needed, OSPM
needs to know in advance which methods will access it -- _DEP provides
OSPM with this
information. While the _DEP keyword may be used to determine start
ordering, only the _REG
method (Section 6.5.4) callbacks can be relied upon to determine
whether a region is accessible at a
given point in time."

So according to the above, _DEP is advisory only and you need _REG.

In theory, because I'm not sure if _REG actually works.

> The problem is that there is no concept of per event dependency. This could have helped
> us figure out if such an interrupt should be enabled or not.
>
> Another solution for operating regions is _REG if FW wants to ignore the event during
> boot. This is the one we are looking into at this moment for non-critical events.
>
> late_init proposed in this patch helps for built-in drivers such as GHES where we do
> not want to ignore events. Since GHES is not an actual device, this has to be solved
> in ACPI.

For built-in things it would be better to have explicit initialization
ordering, eg. initializing both GHES and GED from one initcall in the
right order.

Using different initcall levels for them is sort of OK, but then it is
not quite clear from the code what the specific ordering requirement
is, so please add a comment describing that at least.

Thanks,
Rafael

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

Hi Rafael,

On 5/15/2017 6:59 AM, Rafael J. Wysocki wrote:
> On Mon, May 15, 2017 at 4:36 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> Hi Rafael,
>>
>> On 5/11/2017 10:52 AM, Rafael J. Wysocki wrote:
>>>> OK. I'll reach out to Harb and let's see where the proposal goes.
>>> It looks like this is about operation regions after all, however, so _DEP as is
>>> should be sufficient here.
>>>
>>> There is some limited _DEP support in the ACPI layer, but we were missing
>>> a way to represent those dependencies in the driver core.
>>>
>>> That can be done through device_link objects now, so we may be able to support
>>> _DEP in a more meaningful way, but the cases when _DEP is used for something
>>> different from operation regions in practice need to be treated with caution.
>>>
>>>
>>
>> _DEP could certainly help here. However, _DEP doesn't answer the loose binding question.
>> If one driver is missing in one operating system, _GED driver will never load due
>> to unsatisfied dependency. This forces us into all or none condition. We have operating
>> systems that we need to support and do not have vendor I2C or GPIO drivers. That's why,
>> we are hesitant to place _DEP into ACPI tables.
> 
> _DEP as defined in the spec should be fine still.
> 
> Quoting directly:
> 
> "_DEP evaluates to a package and designates device objects that OSPM
> should assign a higher
> priority in start ordering due to future operation region accesses.
> 
> To increase the likelihood that an SPB operation region handler is
> available when needed, OSPM
> needs to know in advance which methods will access it -- _DEP provides
> OSPM with this
> information. While the _DEP keyword may be used to determine start
> ordering, only the _REG
> method (Section 6.5.4) callbacks can be relied upon to determine
> whether a region is accessible at a
> given point in time."
> 
> So according to the above, _DEP is advisory only and you need _REG.
> 
> In theory, because I'm not sure if _REG actually works.

I have been testing _REG this week. It seems to be working just fine. 
We are transitioning our FW to use _REG. 

Our experience with _DEP in other operating systems is a hard dependency
rather than a soft dependency. We have observed some drivers not loading
if the driver providing the dependency is not present at all. That's why,
I have been taking my time evaluating this. 

> 
>> The problem is that there is no concept of per event dependency. This could have helped
>> us figure out if such an interrupt should be enabled or not.
>>
>> Another solution for operating regions is _REG if FW wants to ignore the event during
>> boot. This is the one we are looking into at this moment for non-critical events.
>>
>> late_init proposed in this patch helps for built-in drivers such as GHES where we do
>> not want to ignore events. Since GHES is not an actual device, this has to be solved
>> in ACPI.
> 
> For built-in things it would be better to have explicit initialization
> ordering, eg. initializing both GHES and GED from one initcall in the
> right order.
> 
> Using different initcall levels for them is sort of OK, but then it is
> not quite clear from the code what the specific ordering requirement
> is, so please add a comment describing that at least.

We are doing two things in this front. The first thing is to change GHES so that
the events that happen before boot are handled properly. Tyler Baicar has a patch
for this. This is the right fix.

The second one is this patch. This one is more of a general solution. We can live
without this patch if Tyler's patch is included at this moment. 

This one is to prevent future problems. I can certainly move the init function around.
Maybe, have one acpi_driver_init that calls ghes_init() followed by ged_init().

Please let me know your preference.

> 
> Thanks,
> Rafael
> 


-- 
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] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

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

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Rafael J. Wysocki]

On Tue, Apr 25, 2017 at 3:43 AM, Sinan Kaya <okaya@codeaurora.org> 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

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Sinan Kaya]

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.

Re: [PATCH] ACPI / GED: use late init to allow other drivers init

[Baicar, Tyler]

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 <okaya@codeaurora.org>
Tested-by: Tyler Baicar <tbaicar@codeaurora.org>

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.