linux-kernel - Re: [PATCH v2 2/2] dmaengine: dw-edma: Do not enable watermark interrupts for HDMA

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: <mhfcw7yuv55me2d7kf6jh3eggzebq6riv5im4nbvx6qrzsg2mr@xpq3srpzemkb>
Date: Fri, 2 Aug 2024 03:34:41 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Mrinmay Sarkar <quic_msarkar@...cinc.com>
Cc: manivannan.sadhasivam@...aro.org, vkoul@...nel.org, 
	quic_shazhuss@...cinc.com, quic_nitegupt@...cinc.com, quic_ramkri@...cinc.com, 
	quic_nayiluri@...cinc.com, quic_krichai@...cinc.com, quic_vbadigan@...cinc.com, 
	stable@...r.kernel.org, Cai Huoqing <cai.huoqing@...ux.dev>, dmaengine@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] dmaengine: dw-edma: Do not enable watermark
 interrupts for HDMA

On Tue, Jul 23, 2024 at 06:49:32PM +0530, Mrinmay Sarkar wrote:
> DW_HDMA_V0_LIE and DW_HDMA_V0_RIE are initialized as BIT(3) and BIT(4)
> respectively in dw_hdma_control enum. But as per HDMA register these
> bits are corresponds to LWIE and RWIE bit i.e local watermark interrupt
> enable and remote watermarek interrupt enable. In linked list mode LWIE
> and RWIE bits only enable the local and remote watermark interrupt.
> 
> Since the watermark interrupts are not used but enabled, this leads to
> spurious interrupts getting generated. So remove the code that enables
> them to avoid generating spurious watermark interrupts.
> 
> And also rename DW_HDMA_V0_LIE to DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE to
> DW_HDMA_V0_RWIE as there is no LIE and RIE bits in HDMA and those bits
> are corresponds to LWIE and RWIE bits.
> 
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> cc: stable@...r.kernel.org
> Signed-off-by: Mrinmay Sarkar <quic_msarkar@...cinc.com>
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index fa89b3a..9ad2e28 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -17,8 +17,8 @@ enum dw_hdma_control {
>  	DW_HDMA_V0_CB					= BIT(0),
>  	DW_HDMA_V0_TCB					= BIT(1),
>  	DW_HDMA_V0_LLP					= BIT(2),
> -	DW_HDMA_V0_LIE					= BIT(3),
> -	DW_HDMA_V0_RIE					= BIT(4),
> +	DW_HDMA_V0_LWIE					= BIT(3),
> +	DW_HDMA_V0_RWIE					= BIT(4),
>  	DW_HDMA_V0_CCS					= BIT(8),
>  	DW_HDMA_V0_LLE					= BIT(9),
>  };
> @@ -195,25 +195,14 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk,
>  static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  {
>  	struct dw_edma_burst *child;
> -	struct dw_edma_chan *chan = chunk->chan;
>  	u32 control = 0, i = 0;
> -	int j;
>  
>  	if (chunk->cb)
>  		control = DW_HDMA_V0_CB;
>  
> -	j = chunk->bursts_alloc;
> -	list_for_each_entry(child, &chunk->burst->list, list) {
> -		j--;
> -		if (!j) {
> -			control |= DW_HDMA_V0_LIE;
> -			if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> -				control |= DW_HDMA_V0_RIE;
> -		}
> -
> +	list_for_each_entry(child, &chunk->burst->list, list)
>  		dw_hdma_v0_write_ll_data(chunk, i++, control, child->sz,
>  					 child->sar, child->dar);
> -	}

Hm, in case of DW EDMA the LIE/RIE flags of the LL entries gets to be
moved to the LIE/RIE flags of the channel context register by the
DMA-engine. In its turn the context register LIE/RIE flags determine
whether the Local and Remote Done/Abort IRQs being raised. So without
the LIE/RIE flags being set in the LL-entries the IRQs won't be raised
and the whole procedure won't work. I have doubts it works differently
in case of HDMA because changing the semantics would cause
implementing additional logic in the DW HDMA RTL-model. Seeing the DW
HDMA IP-core supports the eDMA compatibility mode it would needlessly
expand the controller size. What are the rest of the CONTROL1 register
fields? There must be LIE/RIE flags someplace there for the non-LL
transfers and to preserve the values retrieved from the LL-entries.

Moreover the DW eDMA HW manual has a dedicated chapter called
"Interrupts and Error Handling" with a very demonstrative figures
describing the way the flags work. Does the DW HDMA databook have
something like that?

Please also note, the DW _EDMA_ LIE and RIE flags can be also utilized
for the intermediate IRQ raising, to implement the runtime LL-entries
recycling pattern. The IRQ in that case is called as "watermark" IRQ
in the DW EDMA HW databook, but the flags are still called as just
LIE/RIE.

-Serge(y)

>  
>  	control = DW_HDMA_V0_LLP | DW_HDMA_V0_TCB;
>  	if (!chunk->cb)
> -- 
> 2.7.4
>