From patchwork Fri Aug 14 23:42:21 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 7019861 Return-Path: X-Original-To: patchwork-dmaengine@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 056589F344 for ; Fri, 14 Aug 2015 23:41:38 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D9340206E0 for ; Fri, 14 Aug 2015 23:41:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AB12F206DE for ; Fri, 14 Aug 2015 23:41:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753495AbbHNXld (ORCPT ); Fri, 14 Aug 2015 19:41:33 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:46713 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753273AbbHNXlW (ORCPT ); Fri, 14 Aug 2015 19:41:22 -0400 Received: from avalon.localnet (85-23-193-79.bb.dnainternet.fi [85.23.193.79]) by galahad.ideasonboard.com (Postfix) with ESMTPSA id 0AAAB2005A; Sat, 15 Aug 2015 01:39:56 +0200 (CEST) From: Laurent Pinchart To: Geert Uytterhoeven Cc: dmaengine@vger.kernel.org, linux-sh@vger.kernel.org Subject: Re: Issues with rcar-dmac and sh-sci Date: Sat, 15 Aug 2015 02:42:21 +0300 Message-ID: <1862788.NYkJDoIfhG@avalon> User-Agent: KMail/4.14.8 (Linux/4.0.5-gentoo; KDE/4.14.8; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Sender: dmaengine-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: dmaengine@vger.kernel.org X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham 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 Hi Geert, On Thursday 16 July 2015 20:36:49 Geert Uytterhoeven wrote: > Hi Laurent, > > While working on DMA for R-Car Gen2 using the sh-sci serial driver with > rcar-dmac, I ran into two issues: > > 1. Unlike the old shdmac DMA engine driver, the new rcar-dmac DMA > engine driver does not support resubmitting a DMA descriptor. > I first tried the patch below, until I ran into the race condition, > after which I changed sh-sci to not reuse DMA descriptors. Is reusing descriptors something that the DMA engine API explicitly allows ? > 2. rcar_dmac_chan_get_residue() has: > > static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, > dma_cookie_t cookie) > { > struct rcar_dmac_desc *desc = chan->desc.running; > ... > > /* > * If the cookie doesn't correspond to the currently running > transfer * then the descriptor hasn't been processed yet, and the residue > is * equal to the full descriptor size. > */ > if (cookie != desc->async_tx.cookie) > return desc->size; > > If the cookie doesn't correspond to the currently running transfer, > it returns the full descriptor size of the _currently running > transfer_, not the transfer the cookie corresponds to. How about the following (untested) patch ? From 131968befd5de3400631b879b1574beea27b8239 Mon Sep 17 00:00:00 2001 From: Laurent Pinchart Date: Sat, 15 Aug 2015 01:28:28 +0300 Subject: [PATCH] dmaengine: rcar-dmac: Fix residue reporting for pending descriptors Cookies corresponding to pending transfers have a residue value equal to the full size of the corresponding descriptor. The driver miscomputes that and uses the size of the active descriptor instead. Fix it. Reported-by: Geert Uytterhoeven Signed-off-by: Laurent Pinchart --- drivers/dma/sh/rcar-dmac.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) * In descriptor mode the descriptor running pointer is not maintained > I believe this the reason why the sh-sci driver once thought DMA > transfered 4294967265 (= -31) bytes (for SCIF, descriptor lengths > are either 1 or 32 bytes, and (length) 1 - (residue) 32 = > (transfered) -31). > > Thanks for your comments! > > From 589dbd908a59dba6efc2a78fca24645962235ec2 Mon Sep 17 00:00:00 2001 > From: Geert Uytterhoeven > Date: Tue, 14 Jul 2015 11:27:14 +0200 > Subject: [PATCH] [RFC] dmaengine: rcar-dmac: Allow resubmission of DMA > descriptors > > Unlike the old shdmac DMA engine driver, the new rcar-dmac DMA engine > driver does not support resubmitting a DMA descriptor. If a DMA slave > resubmits a descriptor, the descriptor will be added to the "pending > list", while it wasn't removed from the "wait" list. This will cause > list corruption, leading to an infinite loop in > rcar_dmac_chan_reinit(). > > Find out if the descriptor is reused by looking at the current cookie > valie, and remove it from the other list if needed: > - cookie is initialized to -EBUSY (by rcar-dma) for fresh and properly > recycled descriptors, > - cookie is set to a strict-positive value by dma_cookie_assign() (in > core dmaengine code) on submission, > - cookie is reset to zero by dma_cookie_complete() (in core dmaengine > code) on completion, > > Fix this by removing it from a list if the cookie is not -EBUSY. > > FIXME Unfortunately this is racy: the recycled descriptors are not part > of a list while the DMA descriptor callback is running. > > Signed-off-by: Geert Uytterhoeven > --- > drivers/dma/sh/rcar-dmac.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c > index 11e5003a6cc27b40..92a8fddb025e6729 100644 > --- a/drivers/dma/sh/rcar-dmac.c > +++ b/drivers/dma/sh/rcar-dmac.c > @@ -437,8 +437,20 @@ static dma_cookie_t rcar_dmac_tx_submit(struct > dma_async_tx_descriptor *tx) unsigned long flags; > dma_cookie_t cookie; > > + > spin_lock_irqsave(&chan->lock, flags); > > + if (desc->async_tx.cookie != -EBUSY) { > + /* > + * If the descriptor is reused, it's currently part of a list. > + * Hence it must be removed from that list first, before it can > + * be added to the list of pending requests. > + */ > + dev_dbg(chan->chan.device->dev, "chan%u: resubmit active desc %p\n", > + chan->index, desc); > + list_del(&desc->node); > + } > + > cookie = dma_cookie_assign(tx); > > dev_dbg(chan->chan.device->dev, "chan%u: submit #%d@%p\n", diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c index 7820d07e7bee..a98596a1f12f 100644 --- a/drivers/dma/sh/rcar-dmac.c +++ b/drivers/dma/sh/rcar-dmac.c @@ -1143,19 +1143,43 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, struct rcar_dmac_desc *desc = chan->desc.running; struct rcar_dmac_xfer_chunk *running = NULL; struct rcar_dmac_xfer_chunk *chunk; + enum dma_status status; unsigned int residue = 0; unsigned int dptr = 0; if (!desc) return 0; + + /* + * If the cookie corresponds to a descriptor that has been completed + * there is no residue. The same check has already been performed by the + * caller but without holding the channel lock, so the descriptor could + * now be complete. + */ + status = dma_cookie_status(&chan->chan, cookie, NULL); + if (status == DMA_COMPLETE) + return 0; + /* * If the cookie doesn't correspond to the currently running transfer * then the descriptor hasn't been processed yet, and the residue is * equal to the full descriptor size. */ - if (cookie != desc->async_tx.cookie) - return desc->size; + if (cookie != desc->async_tx.cookie) { + list_for_each_entry(desc, &chan->desc.pending, node) { + if (cookie == desc->async_tx.cookie) + return desc->size; + } + + /* + * No descriptor found for the cookie, there's thus no residue. + * This shouldn't happen if the calling driver passes a correct + * cookie value. + */ + WARN(1, "No descriptor for cookie!"); + return 0; + } /*