From mboxrd@z Thu Jan 1 00:00:00 1970 From: s.hauer@pengutronix.de (Sascha Hauer) Date: Fri, 6 Jan 2012 10:37:52 +0100 Subject: [PATCH 1/2] dmaengine: Fix status handling in imx-dma. In-Reply-To: <1325829389.1540.95.camel@vkoul-udesk3> References: <1325506706-10028-1-git-send-email-javier.martin@vista-silicon.com> <1325829389.1540.95.camel@vkoul-udesk3> Message-ID: <20120106093752.GH5446@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > > --- > > 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 |