Message ID | 20230811081645.1768047-1-yajun.deng@linux.dev |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b824:0:b0:3f2:4152:657d with SMTP id z4csp943141vqi; Fri, 11 Aug 2023 01:43:31 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGiNho+I4EPQbBmZib1fZ8tlfhlu5JEhg3OpIR1mFGp6DpjCyLuwTZUBU84x5g6bUifzkvb X-Received: by 2002:a05:6a20:138b:b0:140:cb66:73c0 with SMTP id hn11-20020a056a20138b00b00140cb6673c0mr1537913pzc.58.1691743411447; Fri, 11 Aug 2023 01:43:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1691743411; cv=none; d=google.com; s=arc-20160816; b=x/aDqP6+V+2AA9NDTt3Hxn2jvP6Mr7QhpfcoMJeFkTp6onnV9JWscffXqeuDlF7D2t 1ObDv8X/YqfWZ8jXH8blXcdvbvRL7EizlaIIcZEkqFsFrc5nbpbVqDTjrgNYPd8jGQau 7QKtfghrc6pSiFXI50cvd8spIjG6r+SAWap2dbqBHVup10Q82GkEo3Wovd8aIat2lNlj gddyi5icizBsVFi476AOinbo8de/z0YWwS6PVJ8nYasZlxFXBBiH9ZYWRH2XCBamZL7a 8M2oVZe7PFJ8EZVLUdN3Kj6oDFV0jXG1v13sYI5ckcnT1/TG2oCfCYg/o0xlZAY3Ue0+ PmTA== 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=vVm1O0Syg4ejjYMdJVi4z6kpg+fEh4jWNr9Mz8DjEBU=; fh=SnawKCBo5eiYDl0u3fVPVQPKJjF7/j1naM/WXKvI3d4=; b=PO04Txt/dDkYc8jI0jPGwnGe6LWG/ZMgtzblthWo2Hv01TC6GmGTN9rzA5qrT0moyy 2pUWSFOqYS5D+N2T/MItCb/UDFTsSfEmUZ7WLJEEOca+i4bVfw4v8O9xSUKUy+AYRvFr +FJecpOcuEODLN5OnAKyZ8VZFxgy9ndzHpoFe6eAUisqjRnC23x0TySyeyuNr4Q79UsY 7SvYzQCNmHOG+/lbUtU2Rf5hQAcxGTsqtv5pCrzz6IU02jUg/aSOXBtXlYi2Zf+GhRMU PtfXL7G14LloLbH/357FzrtlqFx2x8UrInJI2JHbhD2CCfNUPjMXuR5GOiKUqBnJcCWr riXg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=licdqO+s; 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=linux.dev Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 80-20020a630253000000b00563de9d4edasi3132735pgc.366.2023.08.11.01.43.18; Fri, 11 Aug 2023 01:43:31 -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=@linux.dev header.s=key1 header.b=licdqO+s; 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=linux.dev Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234400AbjHKIRF (ORCPT <rfc822;shaohuahua6@gmail.com> + 99 others); Fri, 11 Aug 2023 04:17:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50316 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234473AbjHKIRD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 11 Aug 2023 04:17:03 -0400 Received: from out-94.mta1.migadu.com (out-94.mta1.migadu.com [95.215.58.94]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 93FC126B2 for <linux-kernel@vger.kernel.org>; Fri, 11 Aug 2023 01:17:01 -0700 (PDT) X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1691741819; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=vVm1O0Syg4ejjYMdJVi4z6kpg+fEh4jWNr9Mz8DjEBU=; b=licdqO+sDYnJfKHnR0CE+bAjXPWEEG0jX68sg1wtWqG9HnP5fShB2CEZBloNS31eVOXP9E +kIno0g5qZCfGEgn+WQKsQHHQTa/fSCbLqGizfA8epUW0w6ze0fIkui5ffD8tgyWtmB3Z6 WPElMr+vxUMdHCYTaFR1DbnbGt8yCmk= From: Yajun Deng <yajun.deng@linux.dev> To: vkoul@kernel.org, bhelgaas@google.com, dave.jiang@intel.com Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, Yajun Deng <yajun.deng@linux.dev> Subject: [PATCH] dmaengine: ioat: fixing the wrong chancnt Date: Fri, 11 Aug 2023 16:16:45 +0800 Message-Id: <20230811081645.1768047-1-yajun.deng@linux.dev> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,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: INBOX X-GMAIL-THRID: 1773921539169364952 X-GMAIL-MSGID: 1773921539169364952 |
Series |
dmaengine: ioat: fixing the wrong chancnt
|
|
Commit Message
Yajun Deng
Aug. 11, 2023, 8:16 a.m. UTC
The chancnt would be updated in __dma_async_device_channel_register(),
but it was assigned in ioat_enumerate_channels(). Therefore chancnt has
the wrong value.
Clear chancnt before calling dma_async_device_register().
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
drivers/dma/ioat/init.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On 8/11/23 01:16, Yajun Deng wrote: > The chancnt would be updated in __dma_async_device_channel_register(), > but it was assigned in ioat_enumerate_channels(). Therefore chancnt has > the wrong value. > > Clear chancnt before calling dma_async_device_register(). > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> Thank you for the patch Yajun. While this may work, it clobbers the chancnt read from the hardware. I think the preferable fix is to move the value read from the hardware in ioat_enumerate_channels() and its current usages to 'struct ioatdma_device' and leave dma->chancnt unchanged in that function so that zeroing it later is not needed. Also, have you tested this patch or is this just from visual inspection? And need a fixes tag. > --- > drivers/dma/ioat/init.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c > index c4602bfc9c74..928fc8a83a36 100644 > --- a/drivers/dma/ioat/init.c > +++ b/drivers/dma/ioat/init.c > @@ -536,8 +536,11 @@ static int ioat_probe(struct ioatdma_device *ioat_dma) > > static int ioat_register(struct ioatdma_device *ioat_dma) > { > - int err = dma_async_device_register(&ioat_dma->dma_dev); > + int err; > + > + ioat_dma->dma_dev.chancnt = 0; > > + err = dma_async_device_register(&ioat_dma->dma_dev); > if (err) { > ioat_disable_interrupts(ioat_dma); > dma_pool_destroy(ioat_dma->completion_pool);
August 11, 2023 at 11:40 PM, "Dave Jiang" <dave.jiang@intel.com> wrote: > > On 8/11/23 01:16, Yajun Deng wrote: > > > > > The chancnt would be updated in __dma_async_device_channel_register(), > > but it was assigned in ioat_enumerate_channels(). Therefore chancnt has > > the wrong value. > > Clear chancnt before calling dma_async_device_register(). > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > > > > Thank you for the patch Yajun. > > While this may work, it clobbers the chancnt read from the hardware. I think the preferable fix is to move the value read from the hardware in ioat_enumerate_channels() and its current usages to 'struct ioatdma_device' and leave dma->chancnt unchanged in that function so that zeroing it later is not needed. > Yes, it's even better. I noticed that chancnt is hardware related in ioat, so I just clear it before calling dma_async_device_register().It would be updated after calling dma_async_device_register(). And it would have the same value with read in ioat_enumerate_channels(). It doesn't seem clobber the chancnt read from the hardware. > Also, have you tested this patch or is this just from visual inspection? > Yes, I tested it. ➜ ~ ls /sys/class/dma dma0chan0 dma1chan0 dma2chan0 dma3chan0 before: ➜ ~ cat /sys/kernel/debug/dmaengine/summary dma0 (0000:00:04.0): number of channels: 2 dma1 (0000:00:04.1): number of channels: 2 dma2 (0000:00:04.2): number of channels: 2 dma3 (0000:00:04.3): number of channels: 2 after: ➜ ~ cat /sys/kernel/debug/dmaengine/summary dma0 (0000:00:04.0): number of channels: 1 dma1 (0000:00:04.1): number of channels: 1 dma2 (0000:00:04.2): number of channels: 1 dma3 (0000:00:04.3): number of channels: 1 > And need a fixes tag. > I've tried to find the commit introduced, it looks like it was introduced from the source. The following commits are related to chancnt: 0bbd5f4e97ff ("[I/OAT]: Driver for the Intel(R) I/OAT DMA engine") device->common.chancnt = ioatdma_read8(device, IOAT_CHANCNT_OFFSET); e38288117c50 ("ioatdma: Remove the wrappers around read(bwl)/write(bwl) in ioatdma") device->common.chancnt = readb(device->reg_base + IOAT_CHANCNT_OFFSET); 584ec22759c0 ("ioat: move to drivers/dma/ioat/") move driver/dma/ioatdma.c to driver/dma/ioat/ f2427e276ffe ("ioat: split ioat_dma_probe into core/version-specific routines") dma->chancnt = readb(device->reg_base + IOAT_CHANCNT_OFFSET); 55f878ec47e3 ("dmaengine: ioatdma: fixup ioatdma_device namings") dma->chancnt = readb(ioat_dma->reg_base + IOAT_CHANCNT_OFFSET); It looks very historic. I'm confused about which one to choose. This is a bug, but it only affects /sys/kernel/debug/dmaengine/summary. So I didn't add a fixes tag. > > > > --- > > drivers/dma/ioat/init.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c > > index c4602bfc9c74..928fc8a83a36 100644 > > --- a/drivers/dma/ioat/init.c > > +++ b/drivers/dma/ioat/init.c > > @@ -536,8 +536,11 @@ static int ioat_probe(struct ioatdma_device *ioat_dma) > > > static int ioat_register(struct ioatdma_device *ioat_dma) > > { > > - int err = dma_async_device_register(&ioat_dma->dma_dev); > > + int err; > > + > > + ioat_dma->dma_dev.chancnt = 0; > > > + err = dma_async_device_register(&ioat_dma->dma_dev); > > if (err) { > > ioat_disable_interrupts(ioat_dma); > > dma_pool_destroy(ioat_dma->completion_pool); > > >
On 8/13/23 01:59, Yajun Deng wrote: > August 11, 2023 at 11:40 PM, "Dave Jiang" <dave.jiang@intel.com> wrote: > > >> >> On 8/11/23 01:16, Yajun Deng wrote: >> >>> >>> The chancnt would be updated in __dma_async_device_channel_register(), >>> but it was assigned in ioat_enumerate_channels(). Therefore chancnt has >>> the wrong value. >>> Clear chancnt before calling dma_async_device_register(). >>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >>> >> >> Thank you for the patch Yajun. >> >> While this may work, it clobbers the chancnt read from the hardware. I think the preferable fix is to move the value read from the hardware in ioat_enumerate_channels() and its current usages to 'struct ioatdma_device' and leave dma->chancnt unchanged in that function so that zeroing it later is not needed. >> > Yes, it's even better. I noticed that chancnt is hardware related in ioat, so I just clear it before calling dma_async_device_register().It would be updated after calling dma_async_device_register(). And it would have > the same value with read in ioat_enumerate_channels(). > It doesn't seem clobber the chancnt read from the hardware. > >> Also, have you tested this patch or is this just from visual inspection? >> > Yes, I tested it. > > ➜ ~ ls /sys/class/dma > dma0chan0 dma1chan0 dma2chan0 dma3chan0 > > before: > ➜ ~ cat /sys/kernel/debug/dmaengine/summary > dma0 (0000:00:04.0): number of channels: 2 > dma1 (0000:00:04.1): number of channels: 2 > dma2 (0000:00:04.2): number of channels: 2 > dma3 (0000:00:04.3): number of channels: 2 > > after: > ➜ ~ cat /sys/kernel/debug/dmaengine/summary > dma0 (0000:00:04.0): number of channels: 1 > dma1 (0000:00:04.1): number of channels: 1 > dma2 (0000:00:04.2): number of channels: 1 > dma3 (0000:00:04.3): number of channels: 1 > > >> And need a fixes tag. >> > I've tried to find the commit introduced, it looks like it was introduced from the source. > The following commits are related to chancnt: > > 0bbd5f4e97ff ("[I/OAT]: Driver for the Intel(R) I/OAT DMA engine") > device->common.chancnt = ioatdma_read8(device, IOAT_CHANCNT_OFFSET); > > e38288117c50 ("ioatdma: Remove the wrappers around read(bwl)/write(bwl) in ioatdma") > device->common.chancnt = readb(device->reg_base + IOAT_CHANCNT_OFFSET); > > 584ec22759c0 ("ioat: move to drivers/dma/ioat/") > move driver/dma/ioatdma.c to driver/dma/ioat/ > > f2427e276ffe ("ioat: split ioat_dma_probe into core/version-specific routines") > dma->chancnt = readb(device->reg_base + IOAT_CHANCNT_OFFSET); > > 55f878ec47e3 ("dmaengine: ioatdma: fixup ioatdma_device namings") > dma->chancnt = readb(ioat_dma->reg_base + IOAT_CHANCNT_OFFSET); > > It looks very historic. I'm confused about which one to choose. > This is a bug, but it only affects /sys/kernel/debug/dmaengine/summary. > So I didn't add a fixes tag. > The issue seems cosmetic and does not impact functionality. Let's just leave it alone then. The driver is very old. > >>> >>> --- >>> drivers/dma/ioat/init.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c >>> index c4602bfc9c74..928fc8a83a36 100644 >>> --- a/drivers/dma/ioat/init.c >>> +++ b/drivers/dma/ioat/init.c >>> @@ -536,8 +536,11 @@ static int ioat_probe(struct ioatdma_device *ioat_dma) >>> > static int ioat_register(struct ioatdma_device *ioat_dma) >>> { >>> - int err = dma_async_device_register(&ioat_dma->dma_dev); >>> + int err; >>> + >>> + ioat_dma->dma_dev.chancnt = 0; >>> > + err = dma_async_device_register(&ioat_dma->dma_dev); >>> if (err) { >>> ioat_disable_interrupts(ioat_dma); >>> dma_pool_destroy(ioat_dma->completion_pool); >>> >> I was thinking of something like this to keep the original functionality the same but let the dma->chancnt setup as intended: diff --git a/drivers/dma/ioat/dma.h b/drivers/dma/ioat/dma.h index 35e06b382603..a180171087a8 100644 --- a/drivers/dma/ioat/dma.h +++ b/drivers/dma/ioat/dma.h @@ -74,6 +74,7 @@ struct ioatdma_device { struct dca_provider *dca; enum ioat_irq_mode irq_mode; u32 cap; + int chancnt; /* shadow version for CB3.3 chan reset errata workaround */ u64 msixtba0; diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c index c4602bfc9c74..9c364e92cb82 100644 --- a/drivers/dma/ioat/init.c +++ b/drivers/dma/ioat/init.c @@ -420,7 +420,7 @@ int ioat_dma_setup_interrupts(struct ioatdma_device *ioat_dma) msix: /* The number of MSI-X vectors should equal the number of channels */ - msixcnt = ioat_dma->dma_dev.chancnt; + msixcnt = ioat_dma->chancnt; for (i = 0; i < msixcnt; i++) ioat_dma->msix_entries[i].entry = i; @@ -511,7 +511,7 @@ static int ioat_probe(struct ioatdma_device *ioat_dma) dma_cap_set(DMA_MEMCPY, dma->cap_mask); dma->dev = &pdev->dev; - if (!dma->chancnt) { + if (!ioat_dma->chancnt) { dev_err(dev, "channel enumeration error\n"); goto err_setup_interrupts; } @@ -567,15 +567,16 @@ static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma) struct device *dev = &ioat_dma->pdev->dev; struct dma_device *dma = &ioat_dma->dma_dev; u8 xfercap_log; + int chancnt; int i; INIT_LIST_HEAD(&dma->channels); - dma->chancnt = readb(ioat_dma->reg_base + IOAT_CHANCNT_OFFSET); - dma->chancnt &= 0x1f; /* bits [4:0] valid */ - if (dma->chancnt > ARRAY_SIZE(ioat_dma->idx)) { + chancnt = readb(ioat_dma->reg_base + IOAT_CHANCNT_OFFSET); + chancnt &= 0x1f; /* bits [4:0] valid */ + if (chancnt > ARRAY_SIZE(ioat_dma->idx)) { dev_warn(dev, "(%d) exceeds max supported channels (%zu)\n", - dma->chancnt, ARRAY_SIZE(ioat_dma->idx)); - dma->chancnt = ARRAY_SIZE(ioat_dma->idx); + chancnt, ARRAY_SIZE(ioat_dma->idx)); + chancnt = ARRAY_SIZE(ioat_dma->idx); } xfercap_log = readb(ioat_dma->reg_base + IOAT_XFERCAP_OFFSET); xfercap_log &= 0x1f; /* bits [4:0] valid */ @@ -583,7 +584,7 @@ static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma) return; dev_dbg(dev, "%s: xfercap = %d\n", __func__, 1 << xfercap_log); - for (i = 0; i < dma->chancnt; i++) { + for (i = 0; i < chancnt; i++) { ioat_chan = kzalloc(sizeof(*ioat_chan), GFP_KERNEL); if (!ioat_chan) break; @@ -596,7 +597,7 @@ static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma) break; } } - dma->chancnt = i; + ioat_dma->chancnt = i; } /**
diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c index c4602bfc9c74..928fc8a83a36 100644 --- a/drivers/dma/ioat/init.c +++ b/drivers/dma/ioat/init.c @@ -536,8 +536,11 @@ static int ioat_probe(struct ioatdma_device *ioat_dma) static int ioat_register(struct ioatdma_device *ioat_dma) { - int err = dma_async_device_register(&ioat_dma->dma_dev); + int err; + + ioat_dma->dma_dev.chancnt = 0; + err = dma_async_device_register(&ioat_dma->dma_dev); if (err) { ioat_disable_interrupts(ioat_dma); dma_pool_destroy(ioat_dma->completion_pool);