Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1628653 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-22 00:30 +0200
    Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-22 00:50 +0200
      Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-22 00:50 +0200
      Re: [PATCH] ACPI / GED: use late init to allow other drivers init "Rafael J. Wysocki" <rafael@kernel.org> - 2017-04-25 01:10 +0200
        Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-25 01:40 +0200
          Re: [PATCH] ACPI / GED: use late init to allow other drivers init Lukas Wunner <lukas@wunner.de> - 2017-04-25 09:10 +0200
            Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-25 18:30 +0200
              Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-28 04:40 +0200
                Re: [PATCH] ACPI / GED: use late init to allow other drivers init "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-11 03:10 +0200
                  Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-05-11 03:50 +0200
              Re: [PATCH] ACPI / GED: use late init to allow other drivers init "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-11 03:00 +0200
                Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-05-11 15:50 +0200
                  Re: [PATCH] ACPI / GED: use late init to allow other drivers init "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-11 17:10 +0200
        Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-25 03:50 +0200
          Re: [PATCH] ACPI / GED: use late init to allow other drivers init "Rafael J. Wysocki" <rafael@kernel.org> - 2017-04-25 13:10 +0200
            Re: [PATCH] ACPI / GED: use late init to allow other drivers init Sinan Kaya <okaya@codeaurora.org> - 2017-04-25 18:30 +0200
    Re: [PATCH] ACPI / GED: use late init to allow other drivers init "Rafael J. Wysocki" <rafael@kernel.org> - 2017-04-22 00:50 +0200
    Re: [PATCH] ACPI / GED: use late init to allow other drivers init "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-04-25 01:00 +0200

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

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

[toc] | [next] | [standalone]

#1628659

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.

[toc] | [prev] | [next] | [standalone]

#1628660

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.

[toc] | [prev] | [next] | [standalone]

#1630112

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

[toc] | [prev] | [next] | [standalone]

#1630120

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.

[toc] | [prev] | [next] | [standalone]

#1630235

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

[toc] | [prev] | [next] | [standalone]

#1630754

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.

[toc] | [prev] | [next] | [standalone]

#1632498

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.

[toc] | [prev] | [next] | [standalone]

#1639135

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

[toc] | [prev] | [next] | [standalone]

#1639147

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.

[toc] | [prev] | [next] | [standalone]

#1639129

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

[toc] | [prev] | [next] | [standalone]

#1639449

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.

[toc] | [prev] | [next] | [standalone]

#1639670

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

[toc] | [prev] | [next] | [standalone]

#1630145

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

[toc] | [prev] | [next] | [standalone]

#1630401

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

[toc] | [prev] | [next] | [standalone]

#1630752

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.

[toc] | [prev] | [next] | [standalone]

#1628661

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

[toc] | [prev] | [next] | [standalone]

#1630102

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web