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;
Date:   Mon, 19 Jun 2023 20:41:05 +0200
From:   =?UTF-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>
To:     Serge Semin <fancer.lancer@gmail.com>
Cc:     Cai Huoqing <cai.huoqing@linux.dev>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Vinod Koul <vkoul@kernel.org>,
        Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>,
        dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Herve Codina <herve.codina@bootlin.com>
Subject: Re: [PATCH 5/9] dmaengine: dw-edma: HDMA: Fix possible race
 condition in remote setup
Message-ID: <20230619204105.620f87e6@kmaincent-XPS-13-7390>
In-Reply-To: <20230619171550.3iyujmbre3dpe5oq@mobilestation.baikal.int>
References: <20230609081654.330857-1-kory.maincent@bootlin.com>
        <20230609081654.330857-6-kory.maincent@bootlin.com>
        <20230619171550.3iyujmbre3dpe5oq@mobilestation.baikal.int>
Organization: bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 19 Jun 2023 20:15:50 +0300
Serge Semin <fancer.lancer@gmail.com> wrote:

> On Fri, Jun 09, 2023 at 10:16:50AM +0200, K=C3=B6ry Maincent wrote:
> > From: Kory Maincent <kory.maincent@bootlin.com>
> >=20
> > When writing the linked list elements and pointer the control need to be
> > written at the end. If the control is written and the SAR and DAR not
> > stored we could face a race condition.
> >=20
> > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> =20
>=20
> Once again. Is this a hypothetical bug or have you actually
> experienced the denoted problem? If you do please describe the
> circumstances, give more details. Otherwise it sounds weird. Here is
> why.
>=20
> DW eDMA HW manual states that the control LL DWORD is indeed supposed
> to be written after the rest of the descriptor DWORDs are written. But
> AFAICS it's only relevant for the LL tree entries recycling. Current
> DW eDMA driver design doesn't truly implement that pattern. Instead
> the DMA transfer is halted at the end of the chunk. Then the engine is
> recharged with the next chunk and execution is started over. So the
> runtime recycling isn't implemented (alas) for which the CB flag of
> the control DWORD needs to be updated only after the rest of the LLI
> fields.

This one is only hypothetical. It appears to me that writing the control
after the configuration of sar and dar is more relevant to prevent race iss=
ues
and should be the usual coding choice. Also you are right saying that it wi=
ll
be relevant only for the LL tree entries recycling.
Simple question from non DMA expert: isn't cyclic DMA mode recycle the LL t=
ree
entries?=20

>=20
> If you described a hypothetical problem then it would be ok to accept
> the change for the sake of consistency but I would have dropped the
> Fixes tag and updated the patch description with more details of the
> race condition you are talking about.

Alright, I will do that.

K=C3=B6ry