From patchwork Mon Jan 16 23:33:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tony Lindgren X-Patchwork-Id: 9519657 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 E332C600C5 for ; Mon, 16 Jan 2017 23:35:11 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CC6FC2830A for ; Mon, 16 Jan 2017 23:35:11 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C0FE128447; Mon, 16 Jan 2017 23:35:11 +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 C78C12830A for ; Mon, 16 Jan 2017 23:35:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751370AbdAPXfK (ORCPT ); Mon, 16 Jan 2017 18:35:10 -0500 Received: from muru.com ([72.249.23.125]:57814 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750996AbdAPXfJ (ORCPT ); Mon, 16 Jan 2017 18:35:09 -0500 Received: from atomide.com (localhost [127.0.0.1]) by muru.com (Postfix) with ESMTPS id BDE6080F1; Mon, 16 Jan 2017 23:34:20 +0000 (UTC) Date: Mon, 16 Jan 2017 15:33:30 -0800 From: Tony Lindgren To: Grygorii Strashko Cc: Dan Williams , Vinod Koul , Bin Liu , 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 , Kevin Hilman , Patrick Titiano , Sergei Shtylyov Subject: Re: [PATCHv2] dmaengine: cppi41: Fix oops in cppi41_runtime_resume Message-ID: <20170116233329.GF7403@atomide.com> References: <20170113180132.9188-1-tony@atomide.com> <20170113190126.GE2630@atomide.com> <20170113215936.GF2630@atomide.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170113215936.GF2630@atomide.com> 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 * Tony Lindgren [170113 14:00]: > * Grygorii Strashko [170113 13:37]: > > > Simplified diff with fix on top of your patch: > > > > > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > > > index ce37a1a..9e9403a 100644 > > > --- a/drivers/dma/cppi41.c > > > +++ b/drivers/dma/cppi41.c > > > @@ -319,12 +319,6 @@ 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 != -EINPROGRESS) && (error < 0)) > > > - dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", > > > - __func__, error); > > > > > > /* This warning should never trigger */ > > > WARN_ON(cdd->is_suspended); > > > @@ -500,7 +494,6 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > > > spin_unlock_irqrestore(&cdd->lock, flags); > > > > > > pm_runtime_mark_last_busy(cdd->ddev.dev); > > > - pm_runtime_put_autosuspend(cdd->ddev.dev); > > > } > > > > > > static u32 get_host_pd0(u32 length) > > > > > > > Ok. this is incorrect in case of USB hub and will just hide the problem > > by incrementing PM runtime usage counter every time USB host is connected :( > > Yeah if anything changes in those two nested loops, the pm_runtime counts > get unbalanced. > > > Once USB hub is connected the PM runtime usage counter will became 1 and stay > > 1 after disconnection. Which means that some descriptor was pushed in Q, but IRQ > > was not triggered. > > > > Don't know how to proceed as I'm not so familiar with musb :( > > I'm now playing with saving the queue manager value and kicking the > PM runtime if we have transfers in progress. Looks like the dma > registers are zero while there are transfers in progress, or at least > I have not yet found any hardware registers that would tell that. Looks like drivers/usb/musb/musb_cppi41.c is not properly terminating dma transactions.. The following additional patch makes things behave without warnings for me. I'll fold the drivers/dma/cppi41.c changes to $subject patch and repost, then will post a separate musb fix for drivers/usb/musb/musb_cppi41.c that avoids the warning after some more investigating. Luckily the warning is harmless in this case as musb and cppi41 are in the same device so the common parent is powered and cppi41 is getting clocked. Regards, Tony 8< --------------------------- --- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c --- a/drivers/dma/cppi41.c +++ b/drivers/dma/cppi41.c @@ -705,19 +705,31 @@ static int cppi41_stop_chan(struct dma_chan *chan) u32 desc_phys; int ret; + ret = pm_runtime_get(cdd->ddev.dev); + if ((ret != -EINPROGRESS) && (ret < 0)) + dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", + __func__, ret); + WARN_ON(cdd->is_suspended); + desc_phys = lower_32_bits(c->desc_phys); desc_num = (desc_phys - cdd->descs_phys) / sizeof(struct cppi41_desc); - if (!cdd->chan_busy[desc_num]) - return 0; + if (!cdd->chan_busy[desc_num]) { + ret = 0; + goto out; + } ret = cppi41_tear_down_chan(c); if (ret) - return ret; + goto out; WARN_ON(!cdd->chan_busy[desc_num]); cdd->chan_busy[desc_num] = NULL; - return 0; +out: + pm_runtime_mark_last_busy(cdd->ddev.dev); + pm_runtime_put_autosuspend(cdd->ddev.dev); + + return ret; } static void cleanup_chans(struct cppi41_dd *cdd) diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c --- a/drivers/usb/musb/musb_cppi41.c +++ b/drivers/usb/musb/musb_cppi41.c @@ -445,9 +445,16 @@ static struct dma_channel *cppi41_dma_channel_allocate(struct dma_controller *c, static void cppi41_dma_channel_release(struct dma_channel *channel) { struct cppi41_dma_channel *cppi41_channel = channel->private_data; + struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep; + int error; trace_musb_cppi41_free(cppi41_channel); if (cppi41_channel->is_allocated) { + error = dmaengine_terminate_all(cppi41_channel->dc); + if (error) + dev_err(hw_ep->musb->controller, + "dma terminate failed: %i\n", + error); cppi41_channel->is_allocated = 0; channel->status = MUSB_DMA_STATUS_FREE; channel->actual_len = 0;