linux-kernel - Re: [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed

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: <20250509111554.770263b7@bootlin.com>
Date: Fri, 9 May 2025 11:15:54 +0200
From: Herve Codina <herve.codina@...tlin.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>
Cc: Qiang Zhao <qiang.zhao@....com>, Shengjiu Wang
 <shengjiu.wang@...il.com>, Xiubo Li <Xiubo.Lee@...il.com>, Fabio Estevam
 <festevam@...il.com>, Nicolin Chen <nicoleotsuka@...il.com>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Jaroslav Kysela
 <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
 linux-arm-kernel@...ts.infradead.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when
 needed

Hi Christophe,

On Fri,  9 May 2025 09:48:44 +0200
Christophe Leroy <christophe.leroy@...roup.eu> wrote:

> When no post-completion processing is expected, don't waste time
> handling useless interrupts.
> 
> Only set QMC_BD_[R/T]X_I and QMC_BD_[R/T]X_UB when a completion
> function is passed in.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
>  drivers/soc/fsl/qe/qmc.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
> index 36c0ccc06151..0a704fd0b1a0 100644
> --- a/drivers/soc/fsl/qe/qmc.c
> +++ b/drivers/soc/fsl/qe/qmc.c
> @@ -474,7 +474,9 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>  	xfer_desc->context = context;
>  
>  	/* Activate the descriptor */
> -	ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB);
> +	ctrl |= QMC_BD_TX_R;
> +	if (complete)
> +		ctrl |= QMC_BD_TX_I | QMC_BD_TX_UB;

Be careful, you don't set the UB bit for all descriptor anymore.
Your goal, is to have interrupts only on some descriptors (those where I
bit is set).

This can lead to issue in the function handling the interrupt.
This function, qmc_chan_write_done(), do the processing according to the
following:
        /*
	 * R bit  UB bit
	 *   0       0  : The BD is free
	 *   1       1  : The BD is in used, waiting for transfer
	 *   0       1  : The BD is in used, waiting for completion
	 *   1       0  : Should not append
	 */
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/soc/fsl/qe/qmc.c#L507

It considers R=0 / UB=0 as a free BD and R=1 / UB=0 as a case that should
not happen.

Both cases are no more correct with your modification.

Have the feeling that UB bit still has to be set even if I bit is not set
in order to have qmc_chan_write_done() looking at all descriptors.

Suppose:
 desc 0, no interrupt
 desc 1, no interrupt
 desc 2, interrupt

When the interrupt for desc 2 is handled, desc 0 and desc 1 are seen with
R=0 and UB=0. As desc 0 is considered as free by qmc_chan_write_done(), it
will never look at desc 2.

>  	wmb(); /* Be sure to flush the descriptor before control update */
>  	qmc_write16(&bd->cbd_sc, ctrl);
>  
> @@ -586,7 +588,9 @@ int qmc_chan_read_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>  		  QMC_BD_RX_AB | QMC_BD_RX_CR);
>  
>  	/* Activate the descriptor */
> -	ctrl |= (QMC_BD_RX_E | QMC_BD_RX_UB);
> +	ctrl |= QMC_BD_RX_E;
> +	if (complete)
> +		ctrl |= QMC_BD_RX_I | QMC_BD_RX_UB;

Exact same comment.


Best regards,
Hervé