* [PATCH 1/6] of: dma: protect list write operation by spin_lock
2013-02-12 10:58 [PATCH 0/6] dma: various fixes in different DMAC drivers Andy Shevchenko
@ 2013-02-12 10:58 ` Andy Shevchenko
2013-02-12 10:58 ` [PATCH 2/6] dmaengine.h: remove redundant else keyword Andy Shevchenko
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2013-02-12 10:58 UTC (permalink / raw)
To: Vinod Koul, linux-kernel, Viresh Kumar; +Cc: 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/6] dmaengine.h: remove redundant else keyword
2013-02-12 10:58 [PATCH 0/6] dma: various fixes in different DMAC drivers Andy Shevchenko
2013-02-12 10:58 ` [PATCH 1/6] of: dma: protect list write operation by spin_lock Andy Shevchenko
@ 2013-02-12 10:58 ` Andy Shevchenko
2013-02-12 15:01 ` Viresh Kumar
2013-02-12 10:58 ` [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking Andy Shevchenko
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2013-02-12 10:58 UTC (permalink / raw)
To: Vinod Koul, linux-kernel, Viresh Kumar; +Cc: 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/6] dmaengine.h: remove redundant else keyword
2013-02-12 10:58 ` [PATCH 2/6] dmaengine.h: remove redundant else keyword Andy Shevchenko
@ 2013-02-12 15:01 ` Viresh Kumar
0 siblings, 0 replies; 15+ messages in thread
From: Viresh Kumar @ 2013-02-12 15:01 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Vinod Koul, linux-kernel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking
2013-02-12 10:58 [PATCH 0/6] dma: various fixes in different DMAC drivers Andy Shevchenko
2013-02-12 10:58 ` [PATCH 1/6] of: dma: protect list write operation by spin_lock Andy Shevchenko
2013-02-12 10:58 ` [PATCH 2/6] dmaengine.h: remove redundant else keyword Andy Shevchenko
@ 2013-02-12 10:58 ` Andy Shevchenko
2013-02-12 15:57 ` Viresh Kumar
2013-02-15 14:52 ` Linus Walleij
2013-02-12 10:58 ` [PATCH 4/6] dma: coh901318_lli: set residue only if dma is in progress Andy Shevchenko
` (3 subsequent siblings)
6 siblings, 2 replies; 15+ messages in thread
From: Andy Shevchenko @ 2013-02-12 10:58 UTC (permalink / raw)
To: Vinod Koul, linux-kernel, Viresh Kumar
Cc: Andy Shevchenko, Linus Walleij, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking
2013-02-12 10:58 ` [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking Andy Shevchenko
@ 2013-02-12 15:57 ` Viresh Kumar
2013-02-15 14:52 ` Linus Walleij
1 sibling, 0 replies; 15+ messages in thread
From: Viresh Kumar @ 2013-02-12 15:57 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Vinod Koul, linux-kernel, Linus Walleij, linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking
2013-02-12 10:58 ` [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking Andy Shevchenko
2013-02-12 15:57 ` Viresh Kumar
@ 2013-02-15 14:52 ` Linus Walleij
2013-02-15 14:56 ` Andy Shevchenko
1 sibling, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2013-02-15 14:52 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Vinod Koul, linux-kernel, Viresh Kumar, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking
2013-02-15 14:52 ` Linus Walleij
@ 2013-02-15 14:56 ` Andy Shevchenko
2013-02-15 20:52 ` Linus Walleij
0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2013-02-15 14:56 UTC (permalink / raw)
To: Linus Walleij
Cc: Andy Shevchenko, Vinod Koul, linux-kernel, Viresh Kumar,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking
2013-02-15 14:56 ` Andy Shevchenko
@ 2013-02-15 20:52 ` Linus Walleij
0 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2013-02-15 20:52 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Vinod Koul, linux-kernel, Viresh Kumar,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/6] dma: coh901318_lli: set residue only if dma is in progress
2013-02-12 10:58 [PATCH 0/6] dma: various fixes in different DMAC drivers Andy Shevchenko
` (2 preceding siblings ...)
2013-02-12 10:58 ` [PATCH 3/6] dma: coh901318_lli: avoid unbalanced locking Andy Shevchenko
@ 2013-02-12 10:58 ` Andy Shevchenko
2013-02-15 14:53 ` Linus Walleij
2013-02-12 10:58 ` [PATCH 5/6] edma: do not waste memory for dma_mask Andy Shevchenko
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2013-02-12 10:58 UTC (permalink / raw)
To: Vinod Koul, linux-kernel, Viresh Kumar
Cc: Andy Shevchenko, Linus Walleij, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/6] edma: do not waste memory for dma_mask
2013-02-12 10:58 [PATCH 0/6] dma: various fixes in different DMAC drivers Andy Shevchenko
` (3 preceding siblings ...)
2013-02-12 10:58 ` [PATCH 4/6] dma: coh901318_lli: set residue only if dma is in progress Andy Shevchenko
@ 2013-02-12 10:58 ` Andy Shevchenko
2013-02-12 10:58 ` [PATCH 6/6] dma: tegra20-apb-dma: remove unnecessary assignment Andy Shevchenko
2013-02-12 16:02 ` [PATCH 0/6] dma: various fixes in different DMAC drivers Viresh Kumar
6 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2013-02-12 10:58 UTC (permalink / raw)
To: Vinod Koul, linux-kernel, Viresh Kumar; +Cc: 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 6/6] dma: tegra20-apb-dma: remove unnecessary assignment
2013-02-12 10:58 [PATCH 0/6] dma: various fixes in different DMAC drivers Andy Shevchenko
` (4 preceding siblings ...)
2013-02-12 10:58 ` [PATCH 5/6] edma: do not waste memory for dma_mask Andy Shevchenko
@ 2013-02-12 10:58 ` Andy Shevchenko
2013-02-13 12:40 ` Laxman Dewangan
2013-02-12 16:02 ` [PATCH 0/6] dma: various fixes in different DMAC drivers Viresh Kumar
6 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2013-02-12 10:58 UTC (permalink / raw)
To: Vinod Koul, linux-kernel, Viresh Kumar; +Cc: 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/6] dma: various fixes in different DMAC drivers
2013-02-12 10:58 [PATCH 0/6] dma: various fixes in different DMAC drivers Andy Shevchenko
` (5 preceding siblings ...)
2013-02-12 10:58 ` [PATCH 6/6] dma: tegra20-apb-dma: remove unnecessary assignment Andy Shevchenko
@ 2013-02-12 16:02 ` Viresh Kumar
6 siblings, 0 replies; 15+ messages in thread
From: Viresh Kumar @ 2013-02-12 16:02 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Vinod Koul, linux-kernel
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>
^ permalink raw reply [flat|nested] 15+ messages in thread