Re: [PATCH] dma: mdc: Correct terminate_all handling

[Vinod Koul <vinod.koul@intel.com>]

On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote:
> From: "Damien.Horsley" <Damien.Horsley@imgtec.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

> 
> Signed-off-by: Damien.Horsley <Damien.Horsley@imgtec.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

> +	/*
> +	 * 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

> +	} 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?

-- 
~Vinod