| 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
| ||
|
Message-ID: <20120106093752.GH5446@pengutronix.de>
Date: Fri, 6 Jan 2012 10:37:52 +0100
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Vinod Koul <vinod.koul@...el.com>
Cc: Javier Martin <javier.martin@...ta-silicon.com>,
linux-kernel@...r.kernel.org, dan.j.williams@...el.com,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] dmaengine: Fix status handling in imx-dma.
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@...ta-silicon.com>
> > ---
> > 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists