[PATCH 0/6] dma: various fixes in different DMAC drivers

[PATCH 0/6] dma: various fixes in different DMAC drivers

[Andy Shevchenko]

This is a patcset of independent fixes of the DMAC drivers. They are simple and
being understandable in their context. 

Andy Shevchenko (6):
  of: dma: protect list write operation by spin_lock
  dmaengine.h: remove redundant else keyword
  dma: coh901318_lli: avoid unbalanced locking
  dma: coh901318_lli: set residue only if dma is in progress
  edma: do not waste memory for dma_mask
  dma: tegra20-apb-dma: remove unnecessary assignment

 drivers/dma/coh901318.c       |    4 +++-
 drivers/dma/coh901318_lli.c   |    2 +-
 drivers/dma/edma.c            |    6 ++++--
 drivers/dma/tegra20-apb-dma.c |    1 -
 drivers/of/dma.c              |    2 ++
 include/linux/dmaengine.h     |    3 +--
 6 files changed, 11 insertions(+), 7 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] of: dma: protect list write operation by spin_lock

[Andy Shevchenko]

It's possible to have an inconsistency in the list due to unprotected operation
on it. The patch adds a proper locking on the list operation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Rob Herring <rob.herring@calxeda.com>
---
 drivers/of/dma.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/dma.c b/drivers/of/dma.c
index 59631b2..1ae6120 100644
--- a/drivers/of/dma.c
+++ b/drivers/of/dma.c
@@ -117,7 +117,9 @@ int of_dma_controller_register(struct device_node *np,
 	ofdma->use_count = 0;
 
 	/* Now queue of_dma controller structure in list */
+	spin_lock(&of_dma_lock);
 	list_add_tail(&ofdma->of_dma_controllers, &of_dma_list);
+	spin_unlock(&of_dma_lock);
 
 	return 0;
 }
-- 
1.7.10.4

[PATCH 2/6] dmaengine.h: remove redundant else keyword

[Andy Shevchenko]

dmaengine_device_control returns -ENOSYS in case the dma driver doesn't have
such functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/dmaengine.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index bfcdecb..b760b38 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -610,8 +610,7 @@ static inline int dmaengine_device_control(struct dma_chan *chan,
 {
 	if (chan->device->device_control)
 		return chan->device->device_control(chan, cmd, arg);
-	else
-		return -ENOSYS;
+	return -ENOSYS;
 }
 
 static inline int dmaengine_slave_config(struct dma_chan *chan,
-- 
1.7.10.4

Re: [PATCH 2/6] dmaengine.h: remove redundant else keyword

[Viresh Kumar]

On 12 February 2013 16:28, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> dmaengine_device_control returns -ENOSYS in case the dma driver doesn't have
> such functionality.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/dmaengine.h |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index bfcdecb..b760b38 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -610,8 +610,7 @@ static inline int dmaengine_device_control(struct dma_chan *chan,
>  {
>         if (chan->device->device_control)
>                 return chan->device->device_control(chan, cmd, arg);
> -       else
> -               return -ENOSYS;
> +       return -ENOSYS;

I don't know really which one to prefer here :)

Anyway, if you want this one, then get a blank line before your return code.

[PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Andy Shevchenko]

I have not checked the code but there is a possibility that custom pool->lock
is not needed at all due to dma_pool contains it's own lock.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-arm-kernel at lists.infradead.org
---
 drivers/dma/coh901318_lli.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/coh901318_lli.c b/drivers/dma/coh901318_lli.c
index 3e96610..702112d 100644
--- a/drivers/dma/coh901318_lli.c
+++ b/drivers/dma/coh901318_lli.c
@@ -61,7 +61,7 @@ coh901318_lli_alloc(struct coh901318_pool *pool, unsigned int len)
 	dma_addr_t phy;
 
 	if (len == 0)
-		goto err;
+		return NULL;
 
 	spin_lock(&pool->lock);
 
-- 
1.7.10.4

[PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Andy Shevchenko]

I have not checked the code but there is a possibility that custom pool->lock
is not needed at all due to dma_pool contains it's own lock.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/dma/coh901318_lli.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/coh901318_lli.c b/drivers/dma/coh901318_lli.c
index 3e96610..702112d 100644
--- a/drivers/dma/coh901318_lli.c
+++ b/drivers/dma/coh901318_lli.c
@@ -61,7 +61,7 @@ coh901318_lli_alloc(struct coh901318_pool *pool, unsigned int len)
 	dma_addr_t phy;
 
 	if (len == 0)
-		goto err;
+		return NULL;
 
 	spin_lock(&pool->lock);
 
-- 
1.7.10.4

[PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Viresh Kumar]

On 12 February 2013 16:28, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> I have not checked the code but there is a possibility that custom pool->lock
> is not needed at all due to dma_pool contains it's own lock.

Your viewpoint is fine, but we wanted the reason behind this patch
here. Please add appropriate stuff in only in logs.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-arm-kernel at lists.infradead.org
> ---
>  drivers/dma/coh901318_lli.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/coh901318_lli.c b/drivers/dma/coh901318_lli.c
> index 3e96610..702112d 100644
> --- a/drivers/dma/coh901318_lli.c
> +++ b/drivers/dma/coh901318_lli.c
> @@ -61,7 +61,7 @@ coh901318_lli_alloc(struct coh901318_pool *pool, unsigned int len)
>         dma_addr_t phy;
>
>         if (len == 0)
> -               goto err;
> +               return NULL;
>
>         spin_lock(&pool->lock);
>
> --
> 1.7.10.4
>

Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Viresh Kumar]

On 12 February 2013 16:28, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> I have not checked the code but there is a possibility that custom pool->lock
> is not needed at all due to dma_pool contains it's own lock.

Your viewpoint is fine, but we wanted the reason behind this patch
here. Please add appropriate stuff in only in logs.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/dma/coh901318_lli.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/coh901318_lli.c b/drivers/dma/coh901318_lli.c
> index 3e96610..702112d 100644
> --- a/drivers/dma/coh901318_lli.c
> +++ b/drivers/dma/coh901318_lli.c
> @@ -61,7 +61,7 @@ coh901318_lli_alloc(struct coh901318_pool *pool, unsigned int len)
>         dma_addr_t phy;
>
>         if (len == 0)
> -               goto err;
> +               return NULL;
>
>         spin_lock(&pool->lock);
>
> --
> 1.7.10.4
>

[PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Linus Walleij]

On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> I have not checked the code but there is a possibility that custom pool->lock
> is not needed at all due to dma_pool contains it's own lock.

Can you please change the description to something like:

"The coh901318_lli allocator releases a lock on the exit path without
taking it first".

Which is what the patch is actually fixing.

After that it's Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Linus Walleij]

On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> I have not checked the code but there is a possibility that custom pool->lock
> is not needed at all due to dma_pool contains it's own lock.

Can you please change the description to something like:

"The coh901318_lli allocator releases a lock on the exit path without
taking it first".

Which is what the patch is actually fixing.

After that it's Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Andy Shevchenko]

On Fri, Feb 15, 2013 at 4:52 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>
>> I have not checked the code but there is a possibility that custom pool->lock
>> is not needed at all due to dma_pool contains it's own lock.
>
> Can you please change the description to something like:
>
> "The coh901318_lli allocator releases a lock on the exit path without
> taking it first".

In v2 (that by fact already applied) I put following description:

    In case the len is 0 we must return without trying to unlock the
lock that was
    not locked.

Does it fit your suggestion?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Andy Shevchenko]

On Fri, Feb 15, 2013 at 4:52 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>
>> I have not checked the code but there is a possibility that custom pool->lock
>> is not needed at all due to dma_pool contains it's own lock.
>
> Can you please change the description to something like:
>
> "The coh901318_lli allocator releases a lock on the exit path without
> taking it first".

In v2 (that by fact already applied) I put following description:

    In case the len is 0 we must return without trying to unlock the
lock that was
    not locked.

Does it fit your suggestion?

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Linus Walleij]

On Fri, Feb 15, 2013 at 3:56 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 15, 2013 at 4:52 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>
>>> I have not checked the code but there is a possibility that custom pool->lock
>>> is not needed at all due to dma_pool contains it's own lock.
>>
>> Can you please change the description to something like:
>>
>> "The coh901318_lli allocator releases a lock on the exit path without
>> taking it first".
>
> In v2 (that by fact already applied) I put following description:
>
>     In case the len is 0 we must return without trying to unlock the
> lock that was
>     not locked.
>
> Does it fit your suggestion?

Sure, acked-by.

Thanks,
Linus Walleij

Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking

[Linus Walleij]

On Fri, Feb 15, 2013 at 3:56 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 15, 2013 at 4:52 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>
>>> I have not checked the code but there is a possibility that custom pool->lock
>>> is not needed at all due to dma_pool contains it's own lock.
>>
>> Can you please change the description to something like:
>>
>> "The coh901318_lli allocator releases a lock on the exit path without
>> taking it first".
>
> In v2 (that by fact already applied) I put following description:
>
>     In case the len is 0 we must return without trying to unlock the
> lock that was
>     not locked.
>
> Does it fit your suggestion?

Sure, acked-by.

Thanks,
Linus Walleij

[PATCH 4/6] dma: coh901318_lli: set residue only if dma is in progress

[Andy Shevchenko]

When status is DMA_SUCCESS the residue should be zero. Otherwise it's a bug.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-arm-kernel at lists.infradead.org
---
 drivers/dma/coh901318.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
index a2f079a..797940e 100644
--- a/drivers/dma/coh901318.c
+++ b/drivers/dma/coh901318.c
@@ -2355,7 +2355,9 @@ coh901318_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	enum dma_status ret;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
-	/* FIXME: should be conditional on ret != DMA_SUCCESS? */
+	if (ret == DMA_SUCCESS)
+		return ret;
+
 	dma_set_residue(txstate, coh901318_get_bytes_left(chan));
 
 	if (ret == DMA_IN_PROGRESS && cohc->stopped)
-- 
1.7.10.4

[PATCH 4/6] dma: coh901318_lli: set residue only if dma is in progress

[Andy Shevchenko]

When status is DMA_SUCCESS the residue should be zero. Otherwise it's a bug.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/dma/coh901318.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
index a2f079a..797940e 100644
--- a/drivers/dma/coh901318.c
+++ b/drivers/dma/coh901318.c
@@ -2355,7 +2355,9 @@ coh901318_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	enum dma_status ret;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
-	/* FIXME: should be conditional on ret != DMA_SUCCESS? */
+	if (ret == DMA_SUCCESS)
+		return ret;
+
 	dma_set_residue(txstate, coh901318_get_bytes_left(chan));
 
 	if (ret == DMA_IN_PROGRESS && cohc->stopped)
-- 
1.7.10.4

[PATCH 4/6] dma: coh901318_lli: set residue only if dma is in progress

[Linus Walleij]

On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> When status is DMA_SUCCESS the residue should be zero. Otherwise it's a bug.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 4/6] dma: coh901318_lli: set residue only if dma is in progress

[Linus Walleij]

On Tue, Feb 12, 2013 at 11:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> When status is DMA_SUCCESS the residue should be zero. Otherwise it's a bug.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 5/6] edma: do not waste memory for dma_mask

[Andy Shevchenko]

Accordingly to commentary in the platform_device_register_full the memory
allocated for dma_mask will not going to be freed. That's why is better to
assign dma_mask afterwards.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/edma.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 06ea4b8..cd7e328 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -620,13 +620,11 @@ static struct platform_device *pdev0, *pdev1;
 static const struct platform_device_info edma_dev_info0 = {
 	.name = "edma-dma-engine",
 	.id = 0,
-	.dma_mask = DMA_BIT_MASK(32),
 };
 
 static const struct platform_device_info edma_dev_info1 = {
 	.name = "edma-dma-engine",
 	.id = 1,
-	.dma_mask = DMA_BIT_MASK(32),
 };
 
 static int edma_init(void)
@@ -640,6 +638,8 @@ static int edma_init(void)
 			ret = PTR_ERR(pdev0);
 			goto out;
 		}
+		pdev0->dev.dma_mask = &pdev0->dev.coherent_dma_mask;
+		pdev0->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 	}
 
 	if (EDMA_CTLRS == 2) {
@@ -649,6 +649,8 @@ static int edma_init(void)
 			platform_device_unregister(pdev0);
 			ret = PTR_ERR(pdev1);
 		}
+		pdev1->dev.dma_mask = &pdev1->dev.coherent_dma_mask;
+		pdev1->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 	}
 
 out:
-- 
1.7.10.4

[PATCH 6/6] dma: tegra20-apb-dma: remove unnecessary assignment

[Andy Shevchenko]

There is no need to assign 0 to residue, because dma_cookie_status() does this
for us.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/tegra20-apb-dma.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 46e4630..fcee27e 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -769,7 +769,6 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 
 	ret = dma_cookie_status(dc, cookie, txstate);
 	if (ret == DMA_SUCCESS) {
-		dma_set_residue(txstate, 0);
 		spin_unlock_irqrestore(&tdc->lock, flags);
 		return ret;
 	}
-- 
1.7.10.4

Re: [PATCH 6/6] dma: tegra20-apb-dma: remove unnecessary assignment

[Laxman Dewangan]

On Tuesday 12 February 2013 04:28 PM, Andy Shevchenko wrote:
> There is no need to assign 0 to residue, because dma_cookie_status() does this
> for us.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

For this change(6/6), Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

Re: [PATCH 0/6] dma: various fixes in different DMAC drivers

[Viresh Kumar]

On 12 February 2013 16:28, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> This is a patcset of independent fixes of the DMAC drivers. They are simple and
> being understandable in their context.

Once all minor comments i gave are fixed,

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>