linux-kernel - Re: [PATCH v2 1/5] dmaengine: add ep93xx DMA support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4EC37BBE.6030008@metasoft.pl>
Date:	Wed, 16 Nov 2011 10:00:46 +0100
From:	Rafal Prylowski <prylowski@...asoft.pl>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
CC:	Mika Westerberg <mika.westerberg@....fi>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"rmallon@...il.com" <rmallon@...il.com>,
	"vinod.koul@...el.com" <vinod.koul@...el.com>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"lrg@...com" <lrg@...com>
Subject: Re: [PATCH v2 1/5] dmaengine: add ep93xx DMA support

Hello.

> Nice to see someone is trying to get IDE support for the ep93xx into mainline!
> Unfortunately none of my ep93xx hardware supports IDE... :-(
> 

This driver is a result of work of several people, which I've seen on linux-ide
mailing list. I only added this dmaengine support. I really would like to
see it in mainline, but I think it's still not ready for inclusion.

>>  	default:
>> @@ -668,24 +669,28 @@ static void ep93xx_dma_unmap_buffers(str
>>  static void ep93xx_dma_tasklet(unsigned long data)
>>  {
>>  	struct ep93xx_dma_chan *edmac = (struct ep93xx_dma_chan *)data;
>> -	struct ep93xx_dma_desc *desc, *d;
>> -	dma_async_tx_callback callback;
>> -	void *callback_param;
>> +	struct ep93xx_dma_desc *desc = NULL, *d;
>> +	dma_async_tx_callback callback = NULL;
>> +	void *callback_param = NULL;
>>  	LIST_HEAD(list);
>>  
>>  	spin_lock_irq(&edmac->lock);
>> -	desc = ep93xx_dma_get_active(edmac);
>> -	if (desc->complete) {
>> -		edmac->last_completed = desc->txd.cookie;
>> -		list_splice_init(&edmac->active, &list);
>> +	if (!list_empty(&edmac->active)) {
>> +		desc = ep93xx_dma_get_active(edmac);
>> +		if (desc->complete) {
>> +			edmac->last_completed = desc->txd.cookie;
>> +			list_splice_init(&edmac->active, &list);
>> +		}
> 
> It looks like this might actually catch your BUG_ON issue above.

Yes, I only inserted BUG_ON in ep93xx_dma_get_active() to be sure, that
nowhere else in code happens similar problem.
But now I'm not so sure, that I encountered bug in ep93xx_dma.c, or
maybe I'm misusing dmaengine api (calling dmaengine_termiante_all from
invalid context?). I don't have enough knowledge to judge this.

> 
>>  	}
>>  	spin_unlock_irq(&edmac->lock);
>>  
>>  	/* Pick up the next descriptor from the queue */
>>  	ep93xx_dma_advance_work(edmac);
>>  
>> -	callback = desc->txd.callback;
>> -	callback_param = desc->txd.callback_param;
>> +	if (desc) {
>> +		callback = desc->txd.callback;
>> +		callback_param = desc->txd.callback_param;
>> +	}
> 
> These could be moved up to where 'desc' is getting set.  You have already
> verified that the list is not empty and have a valid 'desc' pointer.  Set
> the callback pointers there to remove this extra if (desc) test.
> 

Following is a patch with suggestions applied (patch addressing problem with
incorrect programming of control register is in another mail sent in reply to
Mika Westerberg).

Regards,
Rafal Prylowski.

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -669,24 +669,25 @@ static void ep93xx_dma_tasklet(unsigned 
 {
 	struct ep93xx_dma_chan *edmac = (struct ep93xx_dma_chan *)data;
 	struct ep93xx_dma_desc *desc, *d;
-	dma_async_tx_callback callback;
-	void *callback_param;
+	dma_async_tx_callback callback = NULL;
+	void *callback_param = NULL;
 	LIST_HEAD(list);
 
 	spin_lock_irq(&edmac->lock);
-	desc = ep93xx_dma_get_active(edmac);
-	if (desc->complete) {
-		edmac->last_completed = desc->txd.cookie;
-		list_splice_init(&edmac->active, &list);
+	if (!list_empty(&edmac->active)) {
+		desc = ep93xx_dma_get_active(edmac);
+		if (desc->complete) {
+			edmac->last_completed = desc->txd.cookie;
+			list_splice_init(&edmac->active, &list);
+		}
+		callback = desc->txd.callback;
+		callback_param = desc->txd.callback_param;
 	}
 	spin_unlock_irq(&edmac->lock);
 
 	/* Pick up the next descriptor from the queue */
 	ep93xx_dma_advance_work(edmac);
 
-	callback = desc->txd.callback;
-	callback_param = desc->txd.callback_param;
-
 	/* Now we can release all the chained descriptors */
 	list_for_each_entry_safe(desc, d, &list, node) {
 		/*

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