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:15:50 +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: <20230619171550.3iyujmbre3dpe5oq@mobilestation.baikal.int>
References: <20230609081654.330857-1-kory.maincent@bootlin.com>
 <20230609081654.330857-6-kory.maincent@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230609081654.330857-6-kory.maincent@bootlin.com>
Precedence: bulk

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.

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.

-Serge(y)

> ---
> 
> This patch is fixing a commit which is only in dmaengine tree and not
> merged mainline.
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 0b77ddbe91b5..f28e1671a753 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -162,10 +162,10 @@ static void dw_hdma_v0_write_ll_data(struct dw_edma_chunk *chunk, int i,
>  	} else {
>  		struct dw_hdma_v0_lli __iomem *lli = chunk->ll_region.vaddr.io + ofs;
>  
> -		writel(control, &lli->control);
>  		writel(size, &lli->transfer_size);
>  		writeq(sar, &lli->sar.reg);
>  		writeq(dar, &lli->dar.reg);
> +		writel(control, &lli->control);
>  	}
>  }
>  
> @@ -182,8 +182,8 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk,
>  	} else {
>  		struct dw_hdma_v0_llp __iomem *llp = chunk->ll_region.vaddr.io + ofs;
>  
> -		writel(control, &llp->control);
>  		writeq(pointer, &llp->llp.reg);
> +		writel(control, &llp->control);
>  	}
>  }
>  
> -- 
> 2.25.1
>