Re: [PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[Sascha Hauer]

On Fri, Jan 06, 2012 at 11:26:29AM +0530, Vinod Koul wrote:
> On Mon, 2012-01-02 at 13:18 +0100, Javier Martin wrote:
> > Status must only be changed to DMA_IN_PROGRESS
> > when the DMA transfer has really begun.
> > 
> > However, since this driver lacks of support for
> > multiple descriptors a new flag has to be introduced
> > to avoid the prepare function be called multiple times.
> Thanks this is the right approach to fix this driver
> 
> But this will obviously break any users of this driver as they need to
> call the right APIs now :D
> 
> Sacha: can you check this patch and see which users of this driver will
> break. we need those fixes to go along this patch as well

Which users should break? I just tried with the mxcmmc driver and this
one does not break.

I do not really understand this patch anyway. It changes imxdmac->status
to a write-only variable and introduces a imxdmac->prepared variable
with the same meaning. This patch is a complicated no-op.

What was the original problem? The fact that we used a enum dma_status
with the wrong semantics? In that case I suggest to simply replace this
variable. All we need to track is that we do not enter
imxdma_prep_slave_sg with an already running transfer.

Sascha

> > 
> > Signed-off-by: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/dma/imx-dma.c |   22 +++++++++++++++-------
> >  1 files changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
> > index d99f71c..9a0ac14 100644
> > --- a/drivers/dma/imx-dma.c
> > +++ b/drivers/dma/imx-dma.c
> > @@ -43,6 +43,7 @@ struct imxdma_channel {
> >  	enum dma_status			status;
> >  	int				dma_request;
> >  	struct scatterlist		*sg_list;
> > +	bool				prepared;
> >  };
> >  
> >  #define MAX_DMA_CHANNELS 8
> > @@ -72,6 +73,7 @@ static void imxdma_irq_handler(int channel, void *data)
> >  
> >  	imxdmac->status = DMA_SUCCESS;
> >  	imxdma_handle(imxdmac);
> > +	imxdmac->prepared = false;
> >  }
> >  
> >  static void imxdma_err_handler(int channel, void *data, int error)
> > @@ -80,6 +82,7 @@ static void imxdma_err_handler(int channel, void *data, int error)
> >  
> >  	imxdmac->status = DMA_ERROR;
> >  	imxdma_handle(imxdmac);
> > +	imxdmac->prepared = false;
> >  }
> >  
> >  static void imxdma_progression(int channel, void *data,
> > @@ -89,6 +92,7 @@ static void imxdma_progression(int channel, void *data,
> >  
> >  	imxdmac->status = DMA_SUCCESS;
> >  	imxdma_handle(imxdmac);
> > +	imxdmac->prepared = false;
> >  }
> >  
> >  static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> > @@ -103,6 +107,7 @@ static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> >  	case DMA_TERMINATE_ALL:
> >  		imxdmac->status = DMA_ERROR;
> >  		imx_dma_disable(imxdmac->imxdma_channel);
> > +		imxdmac->prepared = false;
> >  		return 0;
> >  	case DMA_SLAVE_CONFIG:
> >  		if (dmaengine_cfg->direction == DMA_FROM_DEVICE) {
> > @@ -204,7 +209,7 @@ static int imxdma_alloc_chan_resources(struct dma_chan *chan)
> >  	imxdmac->desc.flags = DMA_CTRL_ACK;
> >  
> >  	imxdmac->status = DMA_SUCCESS;
> > -
> > +	imxdmac->prepared = false;
> >  	return 0;
> >  }
> >  
> > @@ -230,11 +235,9 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
> >  	int i, ret, dma_length = 0;
> >  	unsigned int dmamode;
> >  
> > -	if (imxdmac->status == DMA_IN_PROGRESS)
> > +	if (imxdmac->prepared)
> >  		return NULL;
> >  
> > -	imxdmac->status = DMA_IN_PROGRESS;
> > -
> >  	for_each_sg(sgl, sg, sg_len, i) {
> >  		dma_length += sg->length;
> >  	}
> > @@ -264,6 +267,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
> >  	if (ret)
> >  		return NULL;
> >  
> > +	imxdmac->prepared = true;
> > +
> >  	return &imxdmac->desc;
> >  }
> >  
> > @@ -280,9 +285,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
> >  	dev_dbg(imxdma->dev, "%s channel: %d buf_len=%d period_len=%d\n",
> >  			__func__, imxdmac->channel, buf_len, period_len);
> >  
> > -	if (imxdmac->status == DMA_IN_PROGRESS)
> > +	if (imxdmac->prepared)
> >  		return NULL;
> > -	imxdmac->status = DMA_IN_PROGRESS;
> >  
> >  	ret = imx_dma_setup_progression_handler(imxdmac->imxdma_channel,
> >  			imxdma_progression);
> > @@ -325,14 +329,18 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
> >  	if (ret)
> >  		return NULL;
> >  
> > +	imxdmac->prepared = true;
> > +
> >  	return &imxdmac->desc;
> >  }
> >  
> >  static void imxdma_issue_pending(struct dma_chan *chan)
> >  {
> >  	/*
> > -	 * Nothing to do. We only have a single descriptor
> > +	 * Only change status since we have a single descriptor
> >  	 */
> > +	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
> > +	imxdmac->status = DMA_IN_PROGRESS;
> >  }
> >  
> >  static int __init imxdma_probe(struct platform_device *pdev)
> 
> 
> -- 
> ~Vinod
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/