linux-kernel - Re: [PATCH] dma: mdc: Correct terminate

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]

Date:	Mon, 7 Dec 2015 16:59:41 +0000
From:	Damien Horsley <Damien.Horsley@...tec.com>
To:	Vinod Koul <vinod.koul@...el.com>
CC:	<dmaengine@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Dan Williams <dan.j.williams@...el.com>,
	James Hartley <James.Hartley@...tec.com>
Subject: Re: [PATCH] dma: mdc: Correct terminate_all handling

On 05/12/15 08:43, Vinod Koul wrote:
> On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote:
>> From: "Damien.Horsley" <Damien.Horsley@...tec.com>
>>
>> Use of the CANCEL bit in mdc_terminate_all creates an
>> additional 'command done' to appear in the registers (in
>> addition to an interrupt).
>>
>> In addition, there is a potential race between
>> mdc_terminate_all and the irq handler if a transfer
>> completes at the same time as the terminate all (presently
>> this results in an inappropriate warning).
>>
>> To handle these issues, any outstanding 'command done'
>> events are cleared during mdc_terminate_all and the irq
>> handler takes no action when there are no new 'command done'
>> events.
> 
> SO what does 'command done' event represent, and what would be the behaviour
> of ignoring those
> 

The 'command done' event occurs whenever a new descriptor is loaded from
memory, and once when the whole transfer is completed. Use of the CANCEL
bit can cause additional 'command done' events to occur, which need to
be cleared.

>>
>> Signed-off-by: Damien.Horsley <Damien.Horsley@...tec.com>
>> ---
>>  drivers/dma/img-mdc-dma.c | 71 ++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 45 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
>> index 9ca5683..aa09590 100644
>> --- a/drivers/dma/img-mdc-dma.c
>> +++ b/drivers/dma/img-mdc-dma.c
>> @@ -651,6 +651,42 @@ static enum dma_status mdc_tx_status(struct dma_chan *chan,
>>  	return ret;
>>  }
>>  
>> +static unsigned int mdc_get_new_events(struct mdc_chan *mchan)
>> +{
>> +	u32 val, processed, done1, done2;
>> +	unsigned int ret;
>> +
>> +	val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
>> +	processed = (val >> MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) &
>> +		MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK;
> 
> This should be right justfied
> 

Ok

>> +	/*
>> +	 * CMDS_DONE may have incremented between reading CMDS_PROCESSED
>> +	 * and clearing INT_ACTIVE.  Re-read CMDS_PROCESSED to ensure we
>> +	 * didn't miss a command completion.
>> +	 */
>> +	do {
>> +		val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
>> +		done1 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) &
>> +			MDC_CMDS_PROCESSED_CMDS_DONE_MASK;
>> +		val &= ~((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK <<
>> +			  MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) |
>> +			 MDC_CMDS_PROCESSED_INT_ACTIVE);
>> +		val |= done1 << MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT;
>> +		mdc_chan_writel(mchan, val, MDC_CMDS_PROCESSED);
>> +		val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
>> +		done2 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) &
>> +			MDC_CMDS_PROCESSED_CMDS_DONE_MASK;
> 
> this is very hard to read, can you give some empty lines on logical blocks
>

Ok

>> +	} while (done1 != done2);
>> +
>> +	if (done1 >= processed)
>> +		ret = done1 - processed;
>> +	else
>> +		ret = ((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK + 1) -
>> +			processed) + done1;
>> +
>> +	return ret;
>> +}
>> +
>>  static int mdc_terminate_all(struct dma_chan *chan)
>>  {
>>  	struct mdc_chan *mchan = to_mdc_chan(chan);
>> @@ -667,6 +703,8 @@ static int mdc_terminate_all(struct dma_chan *chan)
>>  	mchan->desc = NULL;
>>  	vchan_get_all_descriptors(&mchan->vc, &head);
>>  
>> +	mdc_get_new_events(mchan);
>> +
> 
> Only getting?
> 

Yes, the extra 'command done' events need to be ignored.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/