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: <4f7dafa5-ba2a-f06f-0fff-8251969b283a@quicinc.com>
Date: Mon, 5 Aug 2024 19:34:46 +0530
From: Mrinmay Sarkar <quic_msarkar@...cinc.com>
To: Serge Semin <fancer.lancer@...il.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 8/2/2024 6:04 AM, Serge Semin wrote:
> 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)
Yes, you are right LIE/RIE flags need to be set without that the IRQs
won't be raised in case of DW EDMA.
But in DW HDMA case there in no such LIE/RIE flags and these particular
bits has been mapped to LWIE and RWIE flags and these are used to enable
watermark interrupt only.
There is no LIE/RIE fields in HDMA_CONTROL1_OFF_WRCH register fields
the same is present in EDMA CONTROL1 register.

DW HDMA has INT_SETUP register and it has LSIE/RSIE, LAIE/RAIE fields
those are enabling Local and Remote Stop/Abort IRQs in LL mode.

yes DW HDMA data book also have figures in "Interrupts and Error Handling"
section and there is no LIE/RIE flags and it is replaced with LWIE/RWIE 
flags
as I mentioned above.

Thanks,
Mrinmay

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