| Message ID | 1492813704-32280-1-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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.
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.
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);
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 -- 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
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
> 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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 >
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?
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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
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
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 -- 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
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
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 -- 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
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 >
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);
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(-)