* [PATCH] dmaengine: ioat: fixing the wrong chancnt @ 2023-08-11 8:16 Yajun Deng 2023-08-11 15:40 ` Dave Jiang 0 siblings, 1 reply; 4+ messages in thread From: Yajun Deng @ 2023-08-11 8:16 UTC (permalink / raw) To: vkoul, bhelgaas, dave.jiang; +Cc: dmaengine, linux-kernel, Yajun Deng 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(-) 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); -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] dmaengine: ioat: fixing the wrong chancnt 2023-08-11 8:16 [PATCH] dmaengine: ioat: fixing the wrong chancnt Yajun Deng @ 2023-08-11 15:40 ` Dave Jiang 2023-08-13 8:59 ` Yajun Deng 0 siblings, 1 reply; 4+ messages in thread From: Dave Jiang @ 2023-08-11 15:40 UTC (permalink / raw) To: Yajun Deng, vkoul, bhelgaas; +Cc: dmaengine, linux-kernel 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); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dmaengine: ioat: fixing the wrong chancnt 2023-08-11 15:40 ` Dave Jiang @ 2023-08-13 8:59 ` Yajun Deng 2023-08-14 15:06 ` Dave Jiang 0 siblings, 1 reply; 4+ messages in thread From: Yajun Deng @ 2023-08-13 8:59 UTC (permalink / raw) To: Dave Jiang, vkoul, bhelgaas; +Cc: dmaengine, linux-kernel 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); > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dmaengine: ioat: fixing the wrong chancnt 2023-08-13 8:59 ` Yajun Deng @ 2023-08-14 15:06 ` Dave Jiang 0 siblings, 0 replies; 4+ messages in thread From: Dave Jiang @ 2023-08-14 15:06 UTC (permalink / raw) To: Yajun Deng, vkoul, bhelgaas; +Cc: dmaengine, linux-kernel 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; } /** ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-08-14 15:08 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-11 8:16 [PATCH] dmaengine: ioat: fixing the wrong chancnt Yajun Deng 2023-08-11 15:40 ` Dave Jiang 2023-08-13 8:59 ` Yajun Deng 2023-08-14 15:06 ` Dave Jiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).