Re: [PATCH] ARM: OMAP2+: omap_device: add fail hook for runtime_pmwhen bad data is detected
From: Nishanth Menon
Date: Wed Dec 04 2013 - 06:33:16 EST
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@xxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Nishanth Menon <nm@xxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/