Message ID | 20230526105434.14959-1-ilpo.jarvinen@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp384528vqr; Fri, 26 May 2023 04:08:42 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6oCXKvytEb/T2NzD6qqBaf3jkOt6iMok7VmKmtGigGTEqi8wb9uEJ/1gUd2N7sNmKpqv9I X-Received: by 2002:a17:902:7c05:b0:1af:bb99:d590 with SMTP id x5-20020a1709027c0500b001afbb99d590mr1898235pll.7.1685099322311; Fri, 26 May 2023 04:08:42 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685099322; cv=none; d=google.com; s=arc-20160816; b=PEpdnFBH/k3HISfMdeZl97nvab9PK8EzZFtM6wuB3G7j7FAu/YnPY5w0MgO1sqcNRZ 4mAwngD+89/yoQoRwBL3ygSvx3TP6qEms+ro+NPpc0G/vV0JicQ9Hi92jdHjWtT3/TT4 eW8W41jU7xr8vs1BFQtcv3wc3NKYLd0XjmMPBOiWSglQxO+/vB3EdZTYW3p706A6K3rb XsdTz/Ieb/bGgsr2hxYNPlnv8FhFJnkGEJUOJNFS7r/bokqxr0ZWuZ/H/Is6Qf9cNq+P ncZBg5jdROEO367VGOVw7posHazAa0VhZ0DS7I+Tq0ToxIsmEWPvGnTcpMfTpggvTicF xV1w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=gnsgaZUGI2KcP/JKeP15nrBjT+aGurpdcY74kwElrGU=; b=h65EuwZvAe1ICm+o6KfuPS/P5fsojCl4I7jHKFKgBVUtJFuyyDEAFkn6vR9LYKSRtx Ke/ILTeL7yDgohC+TJzoSxdnfxn6SJvEVaZQq0hsKbY2vXLnVpWexOXRXx4lNAVktVbT BpibkBaZKjqmAbtGsMdodhre9ZIuHXsnx+rOCqXw9Qj5A2loPWRzOyA2Rue7sKZi4N7c I5IzvEcE1o1HPq1wpf07mxyjw4WpmWNV/pHM7F/dAD2sBAtr5+2j+vT3E77Tb1qUvA3B ly9DgkZygT6dyLWHW8MpUpFMOt2+VNZzRDuJQ98oVBNK8UPTpxEOYWwGJXeouAhiVy8m vrXw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KlQSrhS7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u7-20020a17090282c700b001ab0c00aec4si235904plz.482.2023.05.26.04.08.21; Fri, 26 May 2023 04:08:42 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KlQSrhS7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242917AbjEZKzM (ORCPT <rfc822;zhanglyra.2023@gmail.com> + 99 others); Fri, 26 May 2023 06:55:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49456 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229978AbjEZKzK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 26 May 2023 06:55:10 -0400 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 63EA7F7; Fri, 26 May 2023 03:55:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1685098509; x=1716634509; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=gsACS33QqbMepIsxf09vMS4nFee3rE52HroENxd1Rt4=; b=KlQSrhS7Pu5r4Ojt79iRpGYe7Y+93C9EVAINnOiNy+O3qOSipPyurDnD OEVKdfysKjUT2lqtf6Ocu3YW6QOMLcAmE99SA2hCwUPDnfklc4R5A00wf Y9FpzeFm2Erm7yRCk4ecRHzt+kNrU0tA6vbkouAw/IhL4xLHM/twamHS7 uey1CpekNfJM5MSiKNfypeNwwar0vNutsr/sYEGyiDWX28yTlcidaqIe/ J8EHDNDcGPmPjfFUVVd+5JKM892cghSZ/3v2PXq31+Vk05DQ80FIuFmo0 J6K8GkeeaLrDqD9dn82GNLToCsX2pmTgelRuyA9llVX36JfPGv7ASgO1+ w==; X-IronPort-AV: E=McAfee;i="6600,9927,10721"; a="353018168" X-IronPort-AV: E=Sophos;i="6.00,194,1681196400"; d="scan'208";a="353018168" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 May 2023 03:55:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10721"; a="655599733" X-IronPort-AV: E=Sophos;i="6.00,193,1681196400"; d="scan'208";a="655599733" Received: from eandrei-mobl5.ger.corp.intel.com (HELO ijarvine-MOBL2.ger.corp.intel.com) ([10.252.53.213]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 May 2023 03:55:05 -0700 From: =?utf-8?q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com> To: linux-serial@vger.kernel.org, Vinod Koul <vkoul@kernel.org>, Robert Baldyga <r.baldyga@samsung.com>, dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org Cc: =?utf-8?q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>, Richard Tresidder <rtresidd@electromag.com.au>, stable@vger.kernel.org Subject: [PATCH] dmaengine: pl330: Return DMA_PAUSED when transaction is paused Date: Fri, 26 May 2023 13:54:34 +0300 Message-Id: <20230526105434.14959-1-ilpo.jarvinen@linux.intel.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766954706765422282?= X-GMAIL-MSGID: =?utf-8?q?1766954706765422282?= |
Series |
dmaengine: pl330: Return DMA_PAUSED when transaction is paused
|
|
Commit Message
Ilpo Järvinen
May 26, 2023, 10:54 a.m. UTC
pl330_pause() does not set anything to indicate paused condition which causes pl330_tx_status() to return DMA_IN_PROGRESS. This breaks 8250 DMA flush after the fix in commit 57e9af7831dc ("serial: 8250_dma: Fix DMA Rx rearm race"). The function comment for pl330_pause() claims pause is supported but resume is not which is enough for 8250 DMA flush to work as long as DMA status reports DMA_PAUSED when appropriate. Add PAUSED state for descriptor and mark BUSY descriptors with PAUSED in pl330_pause(). Return DMA_PAUSED from pl330_tx_status() when the descriptor is PAUSED. Reported-by: Richard Tresidder <rtresidd@electromag.com.au> Tested-by: Richard Tresidder <rtresidd@electromag.com.au> Fixes: 88987d2c7534 ("dmaengine: pl330: add DMA_PAUSE feature") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/linux-serial/f8a86ecd-64b1-573f-c2fa-59f541083f1a@electromag.com.au/ Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- $ diff -u <(git grep -l -e '\.device_pause' -e '->device_pause') <(git grep -l DMA_PAUSED) ...tells there might a few other drivers which do not properly return DMA_PAUSED status despite having a pause function. drivers/dma/pl330.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
Comments
Linux regression tracking (Thorsten Leemhuis)
June 26, 2023, 11:50 a.m. UTC |
#1
Addressed
Unaddressed
On 26.05.23 12:54, Ilpo Järvinen wrote: > pl330_pause() does not set anything to indicate paused condition which > causes pl330_tx_status() to return DMA_IN_PROGRESS. This breaks 8250 > DMA flush after the fix in commit 57e9af7831dc ("serial: 8250_dma: Fix > DMA Rx rearm race"). The function comment for pl330_pause() claims > pause is supported but resume is not which is enough for 8250 DMA flush > to work as long as DMA status reports DMA_PAUSED when appropriate. > > Add PAUSED state for descriptor and mark BUSY descriptors with PAUSED > in pl330_pause(). Return DMA_PAUSED from pl330_tx_status() when the > descriptor is PAUSED. > > Reported-by: Richard Tresidder <rtresidd@electromag.com.au> > Tested-by: Richard Tresidder <rtresidd@electromag.com.au> > Fixes: 88987d2c7534 ("dmaengine: pl330: add DMA_PAUSE feature") > Cc: stable@vger.kernel.org > Link: https://lore.kernel.org/linux-serial/f8a86ecd-64b1-573f-c2fa-59f541083f1a@electromag.com.au/ > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Ilpo, Vinod, Philipp: what happened to this? It seems this fix for a regression didn't make any progress since it was posted. Or am I missing something? Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. > --- > > $ diff -u <(git grep -l -e '\.device_pause' -e '->device_pause') <(git grep -l DMA_PAUSED) > > ...tells there might a few other drivers which do not properly return > DMA_PAUSED status despite having a pause function. > > drivers/dma/pl330.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 0d9257fbdfb0..daad25f2c498 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -403,6 +403,12 @@ enum desc_status { > * of a channel can be BUSY at any time. > */ > BUSY, > + /* > + * Pause was called while descriptor was BUSY. Due to hardware > + * limitations, only termination is possible for descriptors > + * that have been paused. > + */ > + PAUSED, > /* > * Sitting on the channel work_list but xfer done > * by PL330 core > @@ -2041,7 +2047,7 @@ static inline void fill_queue(struct dma_pl330_chan *pch) > list_for_each_entry(desc, &pch->work_list, node) { > > /* If already submitted */ > - if (desc->status == BUSY) > + if (desc->status == BUSY || desc->status == PAUSED) > continue; > > ret = pl330_submit_req(pch->thread, desc); > @@ -2326,6 +2332,7 @@ static int pl330_pause(struct dma_chan *chan) > { > struct dma_pl330_chan *pch = to_pchan(chan); > struct pl330_dmac *pl330 = pch->dmac; > + struct dma_pl330_desc *desc; > unsigned long flags; > > pm_runtime_get_sync(pl330->ddma.dev); > @@ -2335,6 +2342,10 @@ static int pl330_pause(struct dma_chan *chan) > _stop(pch->thread); > spin_unlock(&pl330->lock); > > + list_for_each_entry(desc, &pch->work_list, node) { > + if (desc->status == BUSY) > + desc->status = PAUSED; > + } > spin_unlock_irqrestore(&pch->lock, flags); > pm_runtime_mark_last_busy(pl330->ddma.dev); > pm_runtime_put_autosuspend(pl330->ddma.dev); > @@ -2425,7 +2436,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, > else if (running && desc == running) > transferred = > pl330_get_current_xferred_count(pch, desc); > - else if (desc->status == BUSY) > + else if (desc->status == BUSY || desc->status == PAUSED) > /* > * Busy but not running means either just enqueued, > * or finished and not yet marked done > @@ -2442,6 +2453,9 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, > case DONE: > ret = DMA_COMPLETE; > break; > + case PAUSED: > + ret = DMA_PAUSED; > + break; > case PREP: > case BUSY: > ret = DMA_IN_PROGRESS;
On 26-05-23, 13:54, Ilpo Järvinen wrote: > pl330_pause() does not set anything to indicate paused condition which > causes pl330_tx_status() to return DMA_IN_PROGRESS. This breaks 8250 > DMA flush after the fix in commit 57e9af7831dc ("serial: 8250_dma: Fix > DMA Rx rearm race"). The function comment for pl330_pause() claims > pause is supported but resume is not which is enough for 8250 DMA flush > to work as long as DMA status reports DMA_PAUSED when appropriate. > > Add PAUSED state for descriptor and mark BUSY descriptors with PAUSED > in pl330_pause(). Return DMA_PAUSED from pl330_tx_status() when the > descriptor is PAUSED. Have you noticed the comment in the code which reads: /* * We don't support DMA_RESUME command because of hardware * limitations, so after pausing the channel we cannot restore * it to active state. We have to terminate channel and setup * DMA transfer again. This pause feature was implemented to * allow safely read residue before channel termination. */ So driver just stops when in pause. Now the commit 57e9af7831dc returns when in progress state, so am not sure how returning Paused would help here? > > Reported-by: Richard Tresidder <rtresidd@electromag.com.au> > Tested-by: Richard Tresidder <rtresidd@electromag.com.au> > Fixes: 88987d2c7534 ("dmaengine: pl330: add DMA_PAUSE feature") > Cc: stable@vger.kernel.org > Link: https://lore.kernel.org/linux-serial/f8a86ecd-64b1-573f-c2fa-59f541083f1a@electromag.com.au/ > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > > $ diff -u <(git grep -l -e '\.device_pause' -e '->device_pause') <(git grep -l DMA_PAUSED) > > ...tells there might a few other drivers which do not properly return > DMA_PAUSED status despite having a pause function. > > drivers/dma/pl330.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 0d9257fbdfb0..daad25f2c498 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -403,6 +403,12 @@ enum desc_status { > * of a channel can be BUSY at any time. > */ > BUSY, > + /* > + * Pause was called while descriptor was BUSY. Due to hardware > + * limitations, only termination is possible for descriptors > + * that have been paused. > + */ > + PAUSED, > /* > * Sitting on the channel work_list but xfer done > * by PL330 core > @@ -2041,7 +2047,7 @@ static inline void fill_queue(struct dma_pl330_chan *pch) > list_for_each_entry(desc, &pch->work_list, node) { > > /* If already submitted */ > - if (desc->status == BUSY) > + if (desc->status == BUSY || desc->status == PAUSED) > continue; > > ret = pl330_submit_req(pch->thread, desc); > @@ -2326,6 +2332,7 @@ static int pl330_pause(struct dma_chan *chan) > { > struct dma_pl330_chan *pch = to_pchan(chan); > struct pl330_dmac *pl330 = pch->dmac; > + struct dma_pl330_desc *desc; > unsigned long flags; > > pm_runtime_get_sync(pl330->ddma.dev); > @@ -2335,6 +2342,10 @@ static int pl330_pause(struct dma_chan *chan) > _stop(pch->thread); > spin_unlock(&pl330->lock); > > + list_for_each_entry(desc, &pch->work_list, node) { > + if (desc->status == BUSY) > + desc->status = PAUSED; > + } > spin_unlock_irqrestore(&pch->lock, flags); > pm_runtime_mark_last_busy(pl330->ddma.dev); > pm_runtime_put_autosuspend(pl330->ddma.dev); > @@ -2425,7 +2436,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, > else if (running && desc == running) > transferred = > pl330_get_current_xferred_count(pch, desc); > - else if (desc->status == BUSY) > + else if (desc->status == BUSY || desc->status == PAUSED) > /* > * Busy but not running means either just enqueued, > * or finished and not yet marked done > @@ -2442,6 +2453,9 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, > case DONE: > ret = DMA_COMPLETE; > break; > + case PAUSED: > + ret = DMA_PAUSED; > + break; > case PREP: > case BUSY: > ret = DMA_IN_PROGRESS; > -- > 2.30.2
On Wed, 5 Jul 2023, Vinod Koul wrote: > On 26-05-23, 13:54, Ilpo Järvinen wrote: > > pl330_pause() does not set anything to indicate paused condition which > > causes pl330_tx_status() to return DMA_IN_PROGRESS. This breaks 8250 > > DMA flush after the fix in commit 57e9af7831dc ("serial: 8250_dma: Fix > > DMA Rx rearm race"). The function comment for pl330_pause() claims > > pause is supported but resume is not which is enough for 8250 DMA flush > > to work as long as DMA status reports DMA_PAUSED when appropriate. > > > > Add PAUSED state for descriptor and mark BUSY descriptors with PAUSED > > in pl330_pause(). Return DMA_PAUSED from pl330_tx_status() when the > > descriptor is PAUSED. > > Have you noticed the comment in the code which reads: > > /* > * We don't support DMA_RESUME command because of hardware > * limitations, so after pausing the channel we cannot restore > * it to active state. We have to terminate channel and setup > * DMA transfer again. This pause feature was implemented to > * allow safely read residue before channel termination. > */ I'm aware of this limitation (and comment) but it's not causing a problem here since serial8250_rx_dma_flush() does not need to call resume, it requires only supporting pause + reading the state/status. > So driver just stops when in pause. It not only stops but keeps claiming it's still not stopped which causes the problem in 8250 code because 8250 DMA code assumes DMA side returns the correct status. > Now the commit 57e9af7831dc returns when in progress state, so am not > sure how returning Paused would help here? In serial8250_rx_dma_flush() 8250 DMA code does this: dmaengine_pause(dma->rxchan); __dma_rx_complete(p); dmaengine_terminate_async(dma->rxchan); As you can see, __dma_rx_complete() would not take that return when called from serial8250_rx_dma_flush() if correct DMA_* status would be returned. The return in __dma_rx_complete() is meant for other paths (as shown in 57e9af7831dc's changelog) but is now currently taken also when called from serial8250_rx_dma_flush() because pl330 keeps returning DMA_IN_PROGRESS instead of DMA_PAUSED. Thus, I created this fix.
On Fri, 26 May 2023 13:54:34 +0300, Ilpo Järvinen wrote: > pl330_pause() does not set anything to indicate paused condition which > causes pl330_tx_status() to return DMA_IN_PROGRESS. This breaks 8250 > DMA flush after the fix in commit 57e9af7831dc ("serial: 8250_dma: Fix > DMA Rx rearm race"). The function comment for pl330_pause() claims > pause is supported but resume is not which is enough for 8250 DMA flush > to work as long as DMA status reports DMA_PAUSED when appropriate. > > [...] Applied, thanks! [1/1] dmaengine: pl330: Return DMA_PAUSED when transaction is paused commit: 85648667b946069b2a3765577c418705c65c2ddf Best regards,
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 0d9257fbdfb0..daad25f2c498 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -403,6 +403,12 @@ enum desc_status { * of a channel can be BUSY at any time. */ BUSY, + /* + * Pause was called while descriptor was BUSY. Due to hardware + * limitations, only termination is possible for descriptors + * that have been paused. + */ + PAUSED, /* * Sitting on the channel work_list but xfer done * by PL330 core @@ -2041,7 +2047,7 @@ static inline void fill_queue(struct dma_pl330_chan *pch) list_for_each_entry(desc, &pch->work_list, node) { /* If already submitted */ - if (desc->status == BUSY) + if (desc->status == BUSY || desc->status == PAUSED) continue; ret = pl330_submit_req(pch->thread, desc); @@ -2326,6 +2332,7 @@ static int pl330_pause(struct dma_chan *chan) { struct dma_pl330_chan *pch = to_pchan(chan); struct pl330_dmac *pl330 = pch->dmac; + struct dma_pl330_desc *desc; unsigned long flags; pm_runtime_get_sync(pl330->ddma.dev); @@ -2335,6 +2342,10 @@ static int pl330_pause(struct dma_chan *chan) _stop(pch->thread); spin_unlock(&pl330->lock); + list_for_each_entry(desc, &pch->work_list, node) { + if (desc->status == BUSY) + desc->status = PAUSED; + } spin_unlock_irqrestore(&pch->lock, flags); pm_runtime_mark_last_busy(pl330->ddma.dev); pm_runtime_put_autosuspend(pl330->ddma.dev); @@ -2425,7 +2436,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, else if (running && desc == running) transferred = pl330_get_current_xferred_count(pch, desc); - else if (desc->status == BUSY) + else if (desc->status == BUSY || desc->status == PAUSED) /* * Busy but not running means either just enqueued, * or finished and not yet marked done @@ -2442,6 +2453,9 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, case DONE: ret = DMA_COMPLETE; break; + case PAUSED: + ret = DMA_PAUSED; + break; case PREP: case BUSY: ret = DMA_IN_PROGRESS;