From patchwork Wed Dec 4 13:37:02 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nishanth Menon X-Patchwork-Id: 3282651 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id F0B13C0D4A for ; Wed, 4 Dec 2013 13:37:55 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8E581201C7 for ; Wed, 4 Dec 2013 13:37:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0D21E2046F for ; Wed, 4 Dec 2013 13:37:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932305Ab3LDNhb (ORCPT ); Wed, 4 Dec 2013 08:37:31 -0500 Received: from bear.ext.ti.com ([192.94.94.41]:54532 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754072Ab3LDNha (ORCPT ); Wed, 4 Dec 2013 08:37:30 -0500 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id rB4Db30Y022150; Wed, 4 Dec 2013 07:37:03 -0600 Received: from DLEE70.ent.ti.com (dlee70.ent.ti.com [157.170.170.113]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id rB4Db3tG005135; Wed, 4 Dec 2013 07:37:03 -0600 Received: from dlep33.itg.ti.com (157.170.170.75) by DLEE70.ent.ti.com (157.170.170.113) with Microsoft SMTP Server id 14.2.342.3; Wed, 4 Dec 2013 07:37:03 -0600 Received: from localhost (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep33.itg.ti.com (8.14.3/8.13.8) with ESMTP id rB4Db3Tq027202; Wed, 4 Dec 2013 07:37:03 -0600 Date: Wed, 4 Dec 2013 07:37:02 -0600 From: Nishanth Menon To: Joel Fernandes CC: Tony Lindgren , Tobias Jakobi , , , Subject: Re: [PATCH] ARM: OMAP2+: omap_device: add fail hook for runtime_pm when bad data is detected Message-ID: <20131204133702.GA7828@kahuna> References: <1386121153-32351-1-git-send-email-nm@ti.com> <529EE305.70001@ti.com> <529F12F4.70304@ti.com> <529F2391.8070202@ti.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <529F2391.8070202@ti.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 > >>> Signed-off-by: Nishanth Menon > >>> --- > >>> 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] Acked-by: Joel Fernandes 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)