[PATCH] dmatest: properly handle duplicate DMA channels

[PATCH] dmatest: properly handle duplicate DMA channels

[Timur Tabi]

Update the the dmatest driver so that it handles duplicate DMA channels
properly.

When a DMA client is notified of an available DMA channel, it must check if it
has already allocated resources for that channel.  If so, it should return
DMA_DUP.  This can happen, for example, if a DMA driver calls
dma_async_device_register() more than once.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/dma/dmatest.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index a08d197..2689d90 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -321,10 +321,15 @@ static void dmatest_cleanup_channel(struct dmatest_chan *dtc)
 
 static enum dma_state_client dmatest_add_channel(struct dma_chan *chan)
 {
-	struct dmatest_chan	*dtc;
+	struct dmatest_chan	*dtc, *_dtc;
 	struct dmatest_thread	*thread;
 	unsigned int		i;
 
+	/* Have we already been told about this channel? */
+	list_for_each_entry_safe(dtc, _dtc, &dmatest_channels, node)
+		if (dtc->chan == chan)
+			return DMA_DUP;
+
 	dtc = kmalloc(sizeof(struct dmatest_chan), GFP_ATOMIC);
 	if (!dtc) {
 		pr_warning("dmatest: No memory for %s\n", chan->dev.bus_id);
-- 
1.5.5

Re: [PATCH] dmatest: properly handle duplicate DMA channels

[Haavard Skinnemoen]

Timur Tabi <timur@freescale.com> wrote:
> Update the the dmatest driver so that it handles duplicate DMA channels
> properly.
> 
> When a DMA client is notified of an available DMA channel, it must check if it
> has already allocated resources for that channel.  If so, it should return
> DMA_DUP.  This can happen, for example, if a DMA driver calls
> dma_async_device_register() more than once.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>

...though I don't really see why you have to use
list_for_entry_safe()...

Thanks a lot!

Haavard

Re: [PATCH] dmatest: properly handle duplicate DMA channels

[Timur Tabi]

Haavard Skinnemoen wrote:

> ...though I don't really see why you have to use
> list_for_entry_safe()...

You use it in dmatest_remove_channel(), so I figured I'd do the same thing.


-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] dmatest: properly handle duplicate DMA channels

[Haavard Skinnemoen]

Timur Tabi <timur@freescale.com> wrote:
> Haavard Skinnemoen wrote:
> 
> > ...though I don't really see why you have to use
> > list_for_entry_safe()...  
> 
> You use it in dmatest_remove_channel(), so I figured I'd do the same thing.

dmatest_remove_channel() may call list_del() on the current entry, so it
has to. As long as you're not altering the list, the _safe() variant
shouldn't be necessary.

Haavard

Re: [PATCH] dmatest: properly handle duplicate DMA channels

[Andrew Morton]

On Thu, 18 Sep 2008 10:21:19 -0500
Timur Tabi <timur@freescale.com> wrote:

> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -321,10 +321,15 @@ static void dmatest_cleanup_channel(struct dmatest_chan *dtc)
>  
>  static enum dma_state_client dmatest_add_channel(struct dma_chan *chan)
>  {
> -	struct dmatest_chan	*dtc;
> +	struct dmatest_chan	*dtc, *_dtc;
>  	struct dmatest_thread	*thread;
>  	unsigned int		i;
>  
> +	/* Have we already been told about this channel? */
> +	list_for_each_entry_safe(dtc, _dtc, &dmatest_channels, node)
> +		if (dtc->chan == chan)
> +			return DMA_DUP;
> +
>  	dtc = kmalloc(sizeof(struct dmatest_chan), GFP_ATOMIC);
>  	if (!dtc) {
>  		pr_warning("dmatest: No memory for %s\n", chan->dev.bus_id);

hm.  A few lines after that GFP_ATOMIC the driver does a GFP_KERNEL
allocation.

One of them is incorrect.  The interface is undocumented (natch), but I
assume that GFP_KERNEL is the one to use here.

--- a/drivers/dma/dmatest.c~a
+++ a/drivers/dma/dmatest.c
@@ -330,7 +330,7 @@ static enum dma_state_client dmatest_add
 		if (dtc->chan == chan)
 			return DMA_DUP;
 
-	dtc = kmalloc(sizeof(struct dmatest_chan), GFP_ATOMIC);
+	dtc = kmalloc(sizeof(struct dmatest_chan), GFP_KERNEL);
 	if (!dtc) {
 		pr_warning("dmatest: No memory for %s\n", chan->dev.bus_id);
 		return DMA_NAK;
_

Re: [PATCH] dmatest: properly handle duplicate DMA channels

[Haavard Skinnemoen]

Andrew Morton <akpm@linux-foundation.org> wrote:
> One of them is incorrect.  The interface is undocumented (natch), but I
> assume that GFP_KERNEL is the one to use here.

You're right -- there's no reason to use GFP_ATOMIC there. I messed
around with various kinds of locks before I realized that it was
already protected by a mutex.

Haavard

Re: [PATCH] dmatest: properly handle duplicate DMA channels

[Dan Williams]

On Fri, Sep 19, 2008 at 4:31 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 18 Sep 2008 10:21:19 -0500
> Timur Tabi <timur@freescale.com> wrote:
>
>> --- a/drivers/dma/dmatest.c
>> +++ b/drivers/dma/dmatest.c
>> @@ -321,10 +321,15 @@ static void dmatest_cleanup_channel(struct dmatest_chan *dtc)
>>
>>  static enum dma_state_client dmatest_add_channel(struct dma_chan *chan)
>>  {
>> -     struct dmatest_chan     *dtc;
>> +     struct dmatest_chan     *dtc, *_dtc;
>>       struct dmatest_thread   *thread;
>>       unsigned int            i;
>>
>> +     /* Have we already been told about this channel? */
>> +     list_for_each_entry_safe(dtc, _dtc, &dmatest_channels, node)
>> +             if (dtc->chan == chan)
>> +                     return DMA_DUP;
>> +
>>       dtc = kmalloc(sizeof(struct dmatest_chan), GFP_ATOMIC);
>>       if (!dtc) {
>>               pr_warning("dmatest: No memory for %s\n", chan->dev.bus_id);
>
> hm.  A few lines after that GFP_ATOMIC the driver does a GFP_KERNEL
> allocation.
>
> One of them is incorrect.  The interface is undocumented (natch), but I
> assume that GFP_KERNEL is the one to use here.
>

The interface is documented, although not the locking (natch), at
include/linux/dmaengine.h:229.  When I fix this up is there a
canonical location to document a callback interface rather than at the
callback's typedef?

Thanks,
Dan

Re: [PATCH] dmatest: properly handle duplicate DMA channels

[Andrew Morton]

On Sat, 20 Sep 2008 14:40:40 -0700 "Dan Williams" <dan.j.williams@intel.com> wrote:

> When I fix this up is there a
> canonical location to document a callback interface rather than at the
> callback's typedef?

Personally I like to see at at the definition site, so that's within
the struct which contains the function pointer.  eg:

struct address_space_operations {
	int (*writepage)(struct page *page, struct writeback_control *wbc);
	int (*readpage)(struct file *, struct page *);
	void (*sync_page)(struct page *);

	/* Write back some dirty pages from this mapping. */
	int (*writepages)(struct address_space *, struct writeback_control *);

	/* Set a page dirty.  Return true if this dirtied it */
	int (*set_page_dirty)(struct page *page);

(sadly incomplete, but we tried)