Messages in this thread | | | Date | Wed, 04 Dec 2013 05:33:08 -0600 | From | Nishanth Menon <> | Subject | Re: [PATCH] ARM: OMAP2+: omap_device: add fail hook for runtime_pm when bad data is detected |
| |
On 12/04/2013 02:08 AM, Joel Fernandes wrote: > On 12/04/2013 07:09 AM, Nishanth Menon wrote: >> Due to the cross dependencies between hwmod for automanaged device >> information for OMAP and dts node definitions, we can run into scenarios >> where the dts node is defined, however it's hwmod entry is yet to be >> added. In these cases: >> a) omap_device does not register a pm_domain (since it cannot find >> hwmod entry). >> b) driver does not know about (a), does a pm_runtime_get_sync which >> never fails >> c) It then tries to do some operation on the device (such as read the >> revision register (as part of probe) without clock or adequate OMAP >> generic PM operation performed for enabling the module. >> >> This causes a crash such as that reported in: >> https://bugzilla.kernel.org/show_bug.cgi?id=66441 >> >> When 'ti,hwmod' is provided in dt node, it is expected that the device >> will not function without the OMAP's power automanagement. Hence, when >> we hit a fail condition (due to hwmod entries not present or other >> similar scenario), fail at pm_domain level due to lack of data, provide >> enough information for it to be fixed, however, it allows for the driver >> to take appropriate measures to prevent crash. >> >> Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> --- >> arch/arm/mach-omap2/omap_device.c | 24 ++++++++++++++++++++++++ >> arch/arm/mach-omap2/omap_device.h | 1 + >> 2 files changed, 25 insertions(+) >> >> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c >> index 53f0735..e0a398c 100644 >> --- a/arch/arm/mach-omap2/omap_device.c >> +++ b/arch/arm/mach-omap2/omap_device.c >> @@ -183,6 +183,10 @@ static int omap_device_build_from_dt(struct platform_device *pdev) >> odbfd_exit1: >> kfree(hwmods); >> odbfd_exit: >> + /* if data/we are at fault.. load up a fail handler */ >> + if (ret) >> + pdev->dev.pm_domain = &omap_device_fail_pm_domain; >> + >> return ret; >> } >> > > Just wondering, can't we just print the warning here instead of registering new > pm_domain callbacks? >
I suggest you might want to read the commit message again.. but lets try once again:
As you see in dmesg log https://bugzilla.kernel.org/attachment.cgi?id=117311 pointed in the bug https://bugzilla.kernel.org/show_bug.cgi?id=66441,
you already have " [ 0.176940] platform 4b501000.aes: Cannot lookup hwmod 'aes' [ 0.177215] platform 480a5000.des: Cannot lookup hwmod 'des'"
Now, printing that warning does not help, as I already explained in the commit log, " >> b) driver does not know about (a), does a pm_runtime_get_sync which >> never fails"
A device node stated it will have hwmod to adequately control it, but in reality, as in this case, it does not. how does printing a warning alone help the driver which is not aware of these? The driver's attempt at pm_runtime_sync should fail, as that is what "ti,hwmod" property controls.
> Concerned that all this LOC may end up being dead code when the "ti,hwmods" > property becomes obsolete anyway.
we detected we have a bug with 3.13-rc2 - this is a fix for kernel (probably a stable candidate too). ti,hwmod property might become eventually obsolete (and we are working towards that), but the functionality that it provides today is necessary for the transition from mixed dt-hwmod world to pure dt world. - remember we are moving from data structure which is used to describe hardware to another which again describes hardware in a different form - the kind of bugs we see now are expected to be fixed for transition to be smooth for everyone.
without providing adequate warnings, bugs like https://bugzilla.kernel.org/show_bug.cgi?id=66441 will need pretty nasty debug.
I hope this helps convince you that error code is worth the LoC.
-- Regards, Nishanth Menon
| |