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.. >>> 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 :) regards, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
