From patchwork Tue Jun 12 09:20:01 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ulf Hansson X-Patchwork-Id: 10459721 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 04E4360348 for ; Tue, 12 Jun 2018 09:20:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F385C286E2 for ; Tue, 12 Jun 2018 09:20:08 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C7213286EB; Tue, 12 Jun 2018 09:20:08 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EC097286E2 for ; Tue, 12 Jun 2018 09:20:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933156AbeFLJUF (ORCPT ); Tue, 12 Jun 2018 05:20:05 -0400 Received: from mail-io0-f195.google.com ([209.85.223.195]:39475 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932497AbeFLJUC (ORCPT ); Tue, 12 Jun 2018 05:20:02 -0400 Received: by mail-io0-f195.google.com with SMTP id f1-v6so27206810ioh.6 for ; Tue, 12 Jun 2018 02:20:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=yGl347KR5nupdMR5xrPT2gLZ6qvPoaXY0uInbDNTC/4=; b=EbulfF0qGpRIkCX1Hb6P5mLNimeG931L68aMjJMMGTl3noVqvvfBMAEgretKi1yMpB uCtgeE6zI+p4D5icl9GBkmL5yZ274e0HcTf0UYNvZuud4Z5sRaQ7prQPvv2eY3EYFFzm jwd7XcnVECTTWzjp6cdNC4n5ba7a5C6MQg7NQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=yGl347KR5nupdMR5xrPT2gLZ6qvPoaXY0uInbDNTC/4=; b=ILQwlDrktRoIwCEwFkWyZwe1KWYbCMUKjXj5fZWV0ARgUNT8dP2I0G9FY0qEa723UH oVT1BRp1MYRMunS5TbU66BuNse8yPAfzrJsmLQUjTHAyyV10zc9YL8C/GbZ6Bg58CagS K9ozVK5cmLGahhExCViH/h8hRwHVIj/o09tUE6Zv/igt8+MbIQlyMkrRoLFSc6LuWvUu GCDLqusD9gdb3cb8g7x0H9ERj5ZyiYtrEBl1uK92nJDQtNqHVcUjavVtIiFm0whvHoUR aVEFzXmVfwBf1H7JmYHDRdJghtdd0pw3iZ0iMq9Ph8gs3vpFyam5nYoZCHswG6QvMp+o doZg== X-Gm-Message-State: APt69E02B82Qy1+r3avTQQTllAGjrnj7SVbH3sYGCBfuqdvYuYa92wJK jd+kdqKKCgQQ1ZCQEcvFl5020VxYOSSmPE0L5TExfQ== X-Google-Smtp-Source: ADUXVKKMtBXP2YxqoevvGdcSJTdyog9Yy9dz5GmWOda2KUiSQhz2dN9MBmoY7tm3eerGFDHdjAtxHhTs1rnXDt8TTMM= X-Received: by 2002:a6b:c6c9:: with SMTP id w192-v6mr2460329iof.131.1528795202079; Tue, 12 Jun 2018 02:20:02 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:c054:0:0:0:0:0 with HTTP; Tue, 12 Jun 2018 02:20:01 -0700 (PDT) In-Reply-To: <20180612082802eucas1p15f07a37953a5c757136f8cacc6e4f76f~3XEyxN3JH3031930319eucas1p1P@eucas1p1.samsung.com> References: <20180611064838.11383-1-m.szyprowski@samsung.com> <20180611095007eucas1p1a1561911d3e4de4b0add0f581cf24498~3EjLRxlS_0947809478eucas1p1s@eucas1p1.samsung.com> <20180612082802eucas1p15f07a37953a5c757136f8cacc6e4f76f~3XEyxN3JH3031930319eucas1p1P@eucas1p1.samsung.com> From: Ulf Hansson Date: Tue, 12 Jun 2018 11:20:01 +0200 Message-ID: Subject: Re: [PATCH] mmc: dw_mmc-exynos: fix potential external abort in resume_noirq() To: Marek Szyprowski Cc: "linux-mmc@vger.kernel.org" , linux-samsung-soc , Jaehoon Chung , Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz Sender: linux-samsung-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Marek, On 12 June 2018 at 10:28, Marek Szyprowski wrote: > Hi Ulf, > > On 2018-06-11 14:24, Ulf Hansson wrote: >> On 11 June 2018 at 11:50, Marek Szyprowski wrote: >>> On 2018-06-11 11:35, Ulf Hansson wrote: >>>> On 11 June 2018 at 08:48, Marek Szyprowski wrote: >>>>> dw_mci_exynos_resume_noirq() performs DWMMC register access without >>>>> ensuring that respective clocks are enabled. This might cause external >>>>> abort on some systems (observed on Exynos5433 based boards). Fix this >>>>> by adding needed prepare_enable/disable_unprepare calls. >>>>> >>>>> Signed-off-by: Marek Szyprowski >>>>> --- >>>>> drivers/mmc/host/dw_mmc-exynos.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c >>>>> index 3164681108ae..6125b68726b0 100644 >>>>> --- a/drivers/mmc/host/dw_mmc-exynos.c >>>>> +++ b/drivers/mmc/host/dw_mmc-exynos.c >>>>> @@ -193,6 +193,9 @@ static int dw_mci_exynos_resume_noirq(struct device *dev) >>>>> struct dw_mci_exynos_priv_data *priv = host->priv; >>>>> u32 clksel; >>>>> >>>>> + clk_prepare_enable(host->biu_clk); >>>>> + clk_prepare_enable(host->ciu_clk); >>>>> + >>>>> if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 || >>>>> priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU) >>>>> clksel = mci_readl(host, CLKSEL64); >>>>> @@ -207,6 +210,9 @@ static int dw_mci_exynos_resume_noirq(struct device *dev) >>>>> mci_writel(host, CLKSEL, clksel); >>>>> } >>>>> >>>>> + clk_disable_unprepare(host->biu_clk); >>>>> + clk_disable_unprepare(host->ciu_clk); >>>>> + >>>>> return 0; >>>>> } >>>>> #else >>>> I looked a little closer and I am wondering if it wouldn't be possible >>>> to use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() instead of >>>> SET_SYSTEM_SLEEP_PM_OPS()? >>>> >>>> Somelike this: >>>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>>> dw_mci_exynos_resume_noirq) >>>> >>>> Then from dw_mci_exynos_resume_noirq() call pm_runtime_force_resume(). >>>> >>>> I think it would simplify the code a bit, as you can rely on the >>>> runtime PM callbacks to deal with clk_prepare_enable() and >>>> clk_disable_unprepare(), unless I am mistaken. >>> This will not fix the problem, because mci_writel() calls in >>> dw_mci_exynos_resume_noirq are done unconditionally, regardless of the >>> controller's runtime pm state. Since commit 1d9174fbc55e after calling >>> pm_runtime_force_resume() there is no guarantee that device is in >>> runtime active state if it was runtime suspended state. >> Yes, because the runtime PM usage count is greater than 1. >> (pm_runtime_get_noresume() is called during probe). >> >> If you want to make this explicit (not relying on ->probe()), one can >> add a ->suspend_noirq() callback and call pm_runtime_get_noresume() in >> it. > > Sorry, but I don't get how this would work. Exactly the same pattern as > you have proposed was already used in s3c-64xx SPI driver and it didn't > work properly (tested on the same SoC as this DW-MMC change). I had to > move register access to runtime resume callback to fix external abort > issue: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e935dba111621bd6a0c5d48e6511a4d9885103b4 Yep, that is a correct solution. > > Here in DW-MMC driver such approach (moving all the code to runtime > resume callback) is not possible because of the potential interrupt storm > caused by the hw bug (that's the reason of using noirq resume callback). I understand. What you need is to run the runtime resume/suspend callbacks in the resume/suspend noirq phase. Moreover, you need to make sure that the runtime resume callback, really becomes invoked during the resume noirq phase, because of the HW bug. I think the below should work. Can you give it a try? It relies on the call pm_runtime_get_noresume(), done during ->probe(). Note that, the driver always keeps the RPM usage count increased, thus preventing runtime suspend during normal execution. Anyway, if this doesn't work, your suggested approach works fine as well. --- drivers/mmc/host/dw_mmc-exynos.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index a84aa3f1ae85..66132f7fceed 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -175,7 +175,9 @@ static int dw_mci_exynos_runtime_resume(struct device *dev) return ret; } +#endif /* CONFIG_PM */ +#ifdef CONFIG_PM_SLEEP /** * dw_mci_exynos_resume_noirq - Exynos-specific resume code * @@ -193,6 +195,8 @@ static int dw_mci_exynos_resume_noirq(struct device *dev) struct dw_mci_exynos_priv_data *priv = host->priv; u32 clksel; + pm_runtime_force_resume(dev); + if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 || priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU) clksel = mci_readl(host, CLKSEL64); @@ -209,9 +213,7 @@ static int dw_mci_exynos_resume_noirq(struct device *dev) return 0; } -#else -#define dw_mci_exynos_resume_noirq NULL -#endif /* CONFIG_PM */ +#endif /* CONFIG_PM_SLEEP */ static void dw_mci_exynos_config_hs400(struct dw_mci *host, u32 timing) { @@ -553,14 +555,11 @@ static int dw_mci_exynos_remove(struct platform_device *pdev) } static const struct dev_pm_ops dw_mci_exynos_pmops = { - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, - pm_runtime_force_resume) + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + dw_mci_exynos_resume_noirq) SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend, dw_mci_exynos_runtime_resume, NULL) - .resume_noirq = dw_mci_exynos_resume_noirq, - .thaw_noirq = dw_mci_exynos_resume_noirq, - .restore_noirq = dw_mci_exynos_resume_noirq, }; static struct platform_driver dw_mci_exynos_pltfm_driver = {