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 <[email protected]>
---
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
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 <[email protected]>
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" <[email protected]> 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 <[email protected]>
> >
>
> 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" <[email protected]> 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 <[email protected]>
>>>
>>
>> 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;
}
/**