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:   Tue, 20 Jun 2023 15:07:37 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     =?utf-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.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: <wqixf5z7yjbcubjhpkormriptrcihcjezjujmwxihdcagzpzom@igfkbjwuwy7e>
References: <20230609081654.330857-1-kory.maincent@bootlin.com>
 <20230609081654.330857-6-kory.maincent@bootlin.com>
 <20230619171550.3iyujmbre3dpe5oq@mobilestation.baikal.int>
 <20230619204105.620f87e6@kmaincent-XPS-13-7390>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230619204105.620f87e6@kmaincent-XPS-13-7390>
Precedence: bulk

On Mon, Jun 19, 2023 at 08:41:05PM +0200, K?ry Maincent wrote:
> 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?ry Maincent wrote:
> > > From: Kory Maincent <kory.maincent@bootlin.com>
> > > 
> > > 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.
> > > 
> > > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>  
> > 
> > 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.
> > 
> > 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 issues
> and should be the usual coding choice. Also you are right saying that it will
> be relevant only for the LL tree entries recycling.
> Simple question from non DMA expert: isn't cyclic DMA mode recycle the LL tree
> entries? 

Ideally the driver should have been designed in the way you say:
define a ring of the Linked List entries and recycle the already used
entries while the already enabled entries are being handled by the
DMA-engine (a similar approach is described in the DW PCIe/eDMA hw
manual). DW eDMA engine CSRs and LLI descriptor provide enough
functionality for that. Alas the driver implementation is more
straightforward:
1. Each DMA-engine config: SG-list, cyclic, interleaved is split up
into the "burst" entries. SG-list entries are directly mapped to the
eDMA "burst" entries. Cyclic iterations are unrolled into the
respective number of eDMA "burst" entries. A similar story with the
interleaved transactions.
2. If there is no enough entries in the Linked-List memory to fully
execute the requested DMA-transfers, then another so called DW eDMA
"chunk" is allocated.
3. DW eDMA engine executes the "chunks" one after another stopping at
the end of each one and recharging the engine with the next "chunk" until
the last one is finished.

It isn't the most effective architecture, but that's how it was
originally developed by Gustavo. Anyway discussing it is a good food
for thoughts for the driver refactoring though.)

-Serge(y)

> 
> > 
> > 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?ry