Re: [PATCH] ARM: OMAP2+: omap_device: add fail hook for runtime_pmwhen bad data is detected
From: Nishanth Menon
Date: Wed Dec 04 2013 - 08:37:43 EST
On 18:14-20131204, Joel Fernandes wrote:
> On 12/04/2013 05:03 PM, Nishanth Menon wrote:
> > 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:
>
> I know what your patch does and what the problem you're trying to solve is.. Was
> just trying to see if there's a better way of doing what you're trying to do..
Thanks for clarifying.
>
> >>> 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.
>
> Why not do the following?
>
> Assign pm_domain as omap_device_pm_domain always regardless of error or not.
>
> Then in the _od_runtime_resume, check if the od or hwmods exists. If not, print
> the warning. That way you don't need to register additional special callbacks
> just to print a warning and will prolly be fewer LoC fwiw.
>
> That may be harder to do and may require additional checks in omap_device_enable
> etc, not sure. In that case, your approach is certainly the next best way. Just
> thought its worth looking into :)
fair enough, The moment we use the generic omap_device_pm_domain, the
remaining code which assumes od will be valid will need checking.. (so,
we got to do that for all functions where usage is present - fine, that
can be done too)[1] - and yes, it will take care of the pm_runtime handling
However, lets look at the side effect, omap_device_pm_domain also
registers generic suspend_noirq and resume_noirq, and _od_suspend_noirq will
also fail -> as a result device will fail to even attempt to suspend.
That IMHO, is a wrong behavior, So, that explains why we'd need a
omap_device_fail_pm_domain. Keeps the error handling completely
seperated from regular code.
[1]
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index 53f0735..029f076 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -173,7 +173,6 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
r->name = dev_name(&pdev->dev);
}
- pdev->dev.pm_domain = &omap_device_pm_domain;
if (device_active) {
omap_device_enable(pdev);
@@ -183,6 +182,7 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
odbfd_exit1:
kfree(hwmods);
odbfd_exit:
+ pdev->dev.pm_domain = &omap_device_pm_domain;
return ret;
}
@@ -267,6 +267,10 @@ int omap_device_get_context_loss_count(struct platform_device *pdev)
u32 ret = 0;
od = to_omap_device(pdev);
+ if (!od) {
+ dev_err(&pdev->dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
if (od->hwmods_cnt)
ret = omap_hwmod_get_context_loss_count(od->hwmods[0]);
@@ -587,6 +591,12 @@ static int _od_runtime_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
int ret;
+ struct omap_device *od = to_omap_device(pdev);
+
+ if (!od) {
+ dev_err(dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
ret = pm_generic_runtime_suspend(dev);
@@ -599,6 +609,12 @@ static int _od_runtime_suspend(struct device *dev)
static int _od_runtime_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct omap_device *od = to_omap_device(pdev);
+
+ if (!od) {
+ dev_err(dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
omap_device_enable(pdev);
@@ -613,6 +629,11 @@ static int _od_suspend_noirq(struct device *dev)
struct omap_device *od = to_omap_device(pdev);
int ret;
+ if (!od) {
+ dev_err(dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
+
/* Don't attempt late suspend on a driver that is not bound */
if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
return 0;
@@ -635,6 +656,11 @@ static int _od_resume_noirq(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct omap_device *od = to_omap_device(pdev);
+ if (!od) {
+ dev_err(dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
+
if (od->flags & OMAP_DEVICE_SUSPENDED) {
od->flags &= ~OMAP_DEVICE_SUSPENDED;
omap_device_enable(pdev);
@@ -704,6 +730,10 @@ int omap_device_enable(struct platform_device *pdev)
struct omap_device *od;
od = to_omap_device(pdev);
+ if (!od) {
+ dev_err(&pdev->dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
if (od->_state == OMAP_DEVICE_STATE_ENABLED) {
dev_warn(&pdev->dev,
@@ -734,6 +764,10 @@ int omap_device_idle(struct platform_device *pdev)
struct omap_device *od;
od = to_omap_device(pdev);
+ if (!od) {
+ dev_err(&pdev->dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
dev_warn(&pdev->dev,
@@ -767,6 +801,11 @@ int omap_device_assert_hardreset(struct platform_device *pdev, const char *name)
int ret = 0;
int i;
+ if (!od) {
+ dev_err(&pdev->dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
+
for (i = 0; i < od->hwmods_cnt; i++) {
ret = omap_hwmod_assert_hardreset(od->hwmods[i], name);
if (ret)
@@ -795,6 +834,11 @@ int omap_device_deassert_hardreset(struct platform_device *pdev,
int ret = 0;
int i;
+ if (!od) {
+ dev_err(&pdev->dev, "%s: Missing od data\n", __func__);
+ return -ENODEV;
+ }
+
for (i = 0; i < od->hwmods_cnt; i++) {
ret = omap_hwmod_deassert_hardreset(od->hwmods[i], name);
if (ret)
--
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/