From patchwork Wed Jan 18 16:53:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tony Lindgren X-Patchwork-Id: 9524279 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 39D6B60113 for ; Wed, 18 Jan 2017 16:53:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2D5FC2860C for ; Wed, 18 Jan 2017 16:53:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1F6CC2861D; Wed, 18 Jan 2017 16:53:28 +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=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI 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 426322860C for ; Wed, 18 Jan 2017 16:53:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751976AbdARQxZ (ORCPT ); Wed, 18 Jan 2017 11:53:25 -0500 Received: from muru.com ([72.249.23.125]:58364 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751929AbdARQxY (ORCPT ); Wed, 18 Jan 2017 11:53:24 -0500 Received: from atomide.com (localhost [127.0.0.1]) by muru.com (Postfix) with ESMTPS id ABAEB8081; Wed, 18 Jan 2017 16:54:01 +0000 (UTC) Date: Wed, 18 Jan 2017 08:53:09 -0800 From: Tony Lindgren To: Bin Liu , Dan Williams , Vinod Koul , Daniel Mack , Felipe Balbi , Johan Hovold , Peter Ujfalusi , Sekhar Nori , Sebastian Andrzej Siewior , dmaengine@vger.kernel.org, linux-usb@vger.kernel.org, linux-omap@vger.kernel.org, Andy Shevchenko , Grygorii Strashko , Kevin Hilman , Patrick Titiano , Sergei Shtylyov Subject: Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume Message-ID: <20170118165308.GC7403@atomide.com> References: <20170117175524.3484-1-tony@atomide.com> <20170118142501.GA10928@uda0271908> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170118142501.GA10928@uda0271908> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: dmaengine-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: dmaengine@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, * Bin Liu [170118 06:26]: > With this v3, I still get -71 error when a device is plugged to a hub to > musb. It doesn't happen though if the device is already plugged to the hub > before attaching the hub to musb. > > [ 177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc > [ 177.719308] usb 1-1: device descriptor read/64, error -71 > [ 178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc I think the -71 issue is a different regression. I've verified that v4.8.y does not have this, but v4.9.y does have it. So far the easiest way for me to reproduce the -71 problem is by plugging a USB drive into a connected hub. If I connect the hub with USB drive already plugged into the hub, it does not happen. With the hub connected musb is already active when the USB drive is plugged in. So I'm now suspecting this -71 regression is not related to runtime PM changes. Bisect and boot and plug devices time I think unless you have better ideas? > And I still get -115 error flooding with thumb drive. > > [ 236.880068] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115 > > I tested with TI AM335x GP EVM. The problems happen on both musb ports. OK. So it's pointless to try to play with the autosuspend timeout. Let's just do a WARN(!pm_runtime_active(cdd->ddev.dev->parent)) there until we have musb_cppi41.c dma calls properly paired and don't have dma requests lingering. Care to try the updated patch below? It won't help with the -71 issue but the $subject issue should be fixed. And you should not see the WARN() trigger with your tests presumably. Regards, Tony 8< ---------------------------- From tony Mon Sep 17 00:00:00 2001 From: Tony Lindgren Date: Mon, 16 Jan 2017 09:52:10 -0800 Subject: [PATCH] dmaengine: cppi41: Fix oops in cppi41_runtime_resume Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") together with recent MUSB changes allowed USB and DMA on BeagleBone to idle when no cable is connected. But looks like few corner case issues still remain. Looks like just by re-plugging USB cable about ten or so times on BeagleBone when configured in USB peripheral mode we can get warnings and eventually trigger an oops in cppi41 DMA: WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+ x28/0x38 [cppi41] ... WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452 push_desc_queue+0x94/0x9c [cppi41] ... Unable to handle kernel NULL pointer dereference at virtual address 00000104 pgd = c0004000 [00000104] *pgd=00000000 Internal error: Oops: 805 [#1] SMP ARM ... [] (cppi41_runtime_resume [cppi41]) from [] (__rpm_callback+0xc0/0x214) [] (__rpm_callback) from [] (rpm_callback+0x20/0x80) [] (rpm_callback) from [] (rpm_resume+0x504/0x78c) [] (rpm_resume) from [] (pm_runtime_work+0x60/0xa8) [] (pm_runtime_work) from [] (process_one_work+0x2b4/0x808) This is because of a race with runtime PM and cppi41_dma_issue_pending() as reported by Alexandre Bailon in earlier set of patches. Based on mailing list discussions we however came to the conclusion that a different fix from Alexandre's fix is needed in order to guarantee that DMA is really active when we try to use it. To fix the issue, we need to add a driver specific flag as we otherwise can have -EINPROGRESS state set by runtime PM and can't rely on pm_runtime_active() to tell us when we can use the DMA. And we need to make sure the DMA transfers get triggered in the queued order. So let's always queue the transfers, then flush the queue from both cppi41_dma_issue_pending() and cppi41_runtime_resume() as suggested by Grygorii Strashko in an earlier example patch. For reference, this is also documented in Documentation/power/runtime_pm.txt in the example at the end of the file as pointed out by Grygorii Strashko . Based on earlier patches from Alexandre Bailon and Grygorii Strashko modified based on testing and what was discussed on the mailing lists. Note that additional patches are still needed depending on how we want to handle the cppi41 runtime PM. So far cppi41 is always coupled with musb driver, and musb guarantees that cppi41 is always active during use. So until we have a better solution, let's just WARN() if the musb parent is not active to avoid pointless EINPROGRESS warnings during USB mass storage enumeration. As cppi41 is properly clocked with musb being active, we can safely do this. Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") Cc: Andy Shevchenko Cc: Bin Liu Cc: Grygorii Strashko Cc: Kevin Hilman Cc: Patrick Titiano Cc: Sergei Shtylyov Reported-by: Alexandre Bailon Signed-off-by: Tony Lindgren --- drivers/dma/cppi41.c | 51 ++++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c --- a/drivers/dma/cppi41.c +++ b/drivers/dma/cppi41.c @@ -153,6 +153,8 @@ struct cppi41_dd { /* context for suspend/resume */ unsigned int dma_tdfdq; + + bool is_suspended; }; #define FIST_COMPLETION_QUEUE 93 @@ -317,12 +319,10 @@ static irqreturn_t cppi41_irq(int irq, void *data) while (val) { u32 desc, len; - int error; - error = pm_runtime_get(cdd->ddev.dev); - if (error < 0) - dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", - __func__, error); + /* Revisit when musb_cppi41.c dma calls are paired */ + WARN(!pm_runtime_active(cdd->ddev.dev->parent), + "%s parent not active?\n", __func__); q_num = __fls(val); val &= ~(1 << q_num); @@ -343,9 +343,6 @@ static irqreturn_t cppi41_irq(int irq, void *data) c->residue = pd_trans_len(c->desc->pd6) - len; dma_cookie_complete(&c->txd); dmaengine_desc_get_callback_invoke(&c->txd, NULL); - - pm_runtime_mark_last_busy(cdd->ddev.dev); - pm_runtime_put_autosuspend(cdd->ddev.dev); } } return IRQ_HANDLED; @@ -457,20 +454,26 @@ static void push_desc_queue(struct cppi41_channel *c) cppi_writel(reg, cdd->qmgr_mem + QMGR_QUEUE_D(c->q_num)); } -static void pending_desc(struct cppi41_channel *c) +/* + * Caller must hold cdd->lock to prevent push_desc_queue() + * getting called out of order. We have both cppi41_dma_issue_pending() + * and cppi41_runtime_resume() call this function. + */ +static void cppi41_run_queue(struct cppi41_dd *cdd) { - struct cppi41_dd *cdd = c->cdd; - unsigned long flags; + struct cppi41_channel *c, *_c; - spin_lock_irqsave(&cdd->lock, flags); - list_add_tail(&c->node, &cdd->pending); - spin_unlock_irqrestore(&cdd->lock, flags); + list_for_each_entry_safe(c, _c, &cdd->pending, node) { + push_desc_queue(c); + list_del(&c->node); + } } static void cppi41_dma_issue_pending(struct dma_chan *chan) { struct cppi41_channel *c = to_cpp41_chan(chan); struct cppi41_dd *cdd = c->cdd; + unsigned long flags; int error; error = pm_runtime_get(cdd->ddev.dev); @@ -482,10 +485,11 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) return; } - if (likely(pm_runtime_active(cdd->ddev.dev))) - push_desc_queue(c); - else - pending_desc(c); + spin_lock_irqsave(&cdd->lock, flags); + list_add_tail(&c->node, &cdd->pending); + if (!cdd->is_suspended) + cppi41_run_queue(cdd); + spin_unlock_irqrestore(&cdd->lock, flags); pm_runtime_mark_last_busy(cdd->ddev.dev); pm_runtime_put_autosuspend(cdd->ddev.dev); @@ -1150,8 +1154,12 @@ static int __maybe_unused cppi41_resume(struct device *dev) static int __maybe_unused cppi41_runtime_suspend(struct device *dev) { struct cppi41_dd *cdd = dev_get_drvdata(dev); + unsigned long flags; + spin_lock_irqsave(&cdd->lock, flags); + cdd->is_suspended = true; WARN_ON(!list_empty(&cdd->pending)); + spin_unlock_irqrestore(&cdd->lock, flags); return 0; } @@ -1159,14 +1167,11 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev) static int __maybe_unused cppi41_runtime_resume(struct device *dev) { struct cppi41_dd *cdd = dev_get_drvdata(dev); - struct cppi41_channel *c, *_c; unsigned long flags; spin_lock_irqsave(&cdd->lock, flags); - list_for_each_entry_safe(c, _c, &cdd->pending, node) { - push_desc_queue(c); - list_del(&c->node); - } + cdd->is_suspended = false; + cppi41_run_queue(cdd); spin_unlock_irqrestore(&cdd->lock, flags); return 0;