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;
MIME-Version: 1.0
Date:   Sun, 13 Aug 2023 08:59:50 +0000
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
From:   "Yajun Deng" <yajun.deng@linux.dev>
Message-ID: <148ae07079b42d834a19b100e18070f50acbcc78@linux.dev>
TLS-Required: No
Subject: Re: [PATCH] dmaengine: ioat: fixing the wrong chancnt
To:     "Dave Jiang" <dave.jiang@intel.com>, vkoul@kernel.org,
        bhelgaas@google.com
Cc:     dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <a93a087d-cfae-a135-999e-ae1976694165@intel.com>
References: <a93a087d-cfae-a135-999e-ae1976694165@intel.com>
 <20230811081645.1768047-1-yajun.deng@linux.dev>
Precedence: bulk

August 11, 2023 at 11:40 PM, "Dave Jiang" <dave.jiang@intel.com> wrote:


>=20
>=20On 8/11/23 01:16, Yajun Deng wrote:
>=20
>=20>=20
>=20> 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>
> >=20
>=20
> Thank you for the patch Yajun.
>=20
>=20While 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_devi=
ce' 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.=20
=20
> Also, have you tested this patch or is this just from visual inspection=
?
>=20
Yes,=20I tested it.

=E2=9E=9C  ~ ls /sys/class/dma
dma0chan0  dma1chan0  dma2chan0  dma3chan0

before:
=E2=9E=9C  ~ 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:
=E2=9E=9C  ~ 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=EF=BC=9A

0bbd5f4e97ff ("[I/OAT]: Driver for the Intel(R) I/OAT DMA engine")
device->common.chancnt =3D ioatdma_read8(device, IOAT_CHANCNT_OFFSET);

e38288117c50 ("ioatdma: Remove the wrappers around read(bwl)/write(bwl) i=
n ioatdma")
device->common.chancnt =3D 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 rout=
ines")
dma->chancnt =3D readb(device->reg_base + IOAT_CHANCNT_OFFSET);

55f878ec47e3 ("dmaengine: ioatdma: fixup ioatdma_device namings")
dma->chancnt =3D readb(ioat_dma->reg_base + IOAT_CHANCNT_OFFSET);

It looks very historic. I'm confused about which one to choose.=20
This=20is a bug, but it only affects /sys/kernel/debug/dmaengine/summary.
So I didn't add a fixes tag.

=20
>=20>=20
>=20> ---
> >  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 *io=
at_dma)
> >  > static int ioat_register(struct ioatdma_device *ioat_dma)
> >  {
> >  - int err =3D dma_async_device_register(&ioat_dma->dma_dev);
> >  + int err;
> >  +
> >  + ioat_dma->dma_dev.chancnt =3D 0;
> >  > + err =3D dma_async_device_register(&ioat_dma->dma_dev);
> >  if (err) {
> >  ioat_disable_interrupts(ioat_dma);
> >  dma_pool_destroy(ioat_dma->completion_pool);
> >
>