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

[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 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 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
>

[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 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 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