linux-kernel - Re: [PATCH] shdmac: Remove misleading TODO comment in dmae_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c1983852-b6ae-484b-989b-56fe0d00a679@linaro.org>
Date: Tue, 2 Dec 2025 12:12:03 +0200
From: Eugen Hristev <eugen.hristev@...aro.org>
To: Amin GATTOUT <amin.gattout@...il.com>, vkoul@...nel.org,
 thomasandreatta2000@...il.com
Cc: dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] shdmac: Remove misleading TODO comment in dmae_set_chcr

On 11/28/25 17:29, Amin GATTOUT wrote:
> The comment suggested that the dmae_is_busy() check in dmae_set_chcr()
> is superfluous and could be removed. However, this check serves as an
> important safety net to prevent configuration of a DMA channel while

I find this a bit odd overall, because apparently nobody checks the
result of dmae_set_chcr() .
So if it is such an important safety check, why is the result never
checked ?
As it looks, the caller doesn't care and continue as usual. The
difference would be that chcr is never actually written if the channel
is busy. Which looks strange. And "unexpected hardware behavior in edge
cases" is quite vague. Do you have a scenario when an issue would happen ?
dmae_set_chcr() gets called on resume() and setup_xfer(). Is it possible
that in fact dmae_set_chcr() is not called correctly then ? Maybe this
chcr should be written at a different time when we are sure the dma is
not busy ?
Or why is it even possible to have the dma busy when calling it ?

Eugen

> it is active. Keeping it helps ensure transfer integrity and avoids
> unexpected hardware behavior in edge cases.
> 
> Signed-off-by: Amin GATTOUT <amin.gattout@...il.com>
> ---
>  drivers/dma/sh/shdmac.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c
> index 603e15102e45..d0e0437ad916 100644
> --- a/drivers/dma/sh/shdmac.c
> +++ b/drivers/dma/sh/shdmac.c
> @@ -243,7 +243,6 @@ static void dmae_init(struct sh_dmae_chan *sh_chan)
>  
>  static int dmae_set_chcr(struct sh_dmae_chan *sh_chan, u32 val)
>  {
> -	/* If DMA is active, cannot set CHCR. TODO: remove this superfluous check */
>  	if (dmae_is_busy(sh_chan))
>  		return -EBUSY;
>