Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41

[Abramo Bagnara <abramo.bagnara@tin.it>]

Jaroslav Kysela ha scritto:
> On Sun, 1 Feb 2004, Abramo Bagnara wrote:
> 
> 
>>Jaroslav Kysela ha scritto:
>>
>>>Modified Files:
>>>	rawmidi.c 
>>>Log Message:
>>>copy_*_user() function cannot be called from spinlock context
>>>
>>>Index: rawmidi.c
>>>===================================================================
>>>RCS file: /cvsroot/alsa/alsa-kernel/core/rawmidi.c,v
>>>retrieving revision 1.40
>>>retrieving revision 1.41
>>>diff -u -r1.40 -r1.41
>>>--- rawmidi.c	23 Oct 2003 14:34:52 -0000	1.40
>>>+++ rawmidi.c	1 Feb 2004 16:34:28 -0000	1.41
>>>@@ -909,10 +909,11 @@
>>> 		if (kernel) {
>>> 			memcpy(buf + result, runtime->buffer + runtime->appl_ptr, count1);
>>> 		} else {
>>>+			spin_unlock_irqrestore(&runtime->lock, flags);
>>> 			if (copy_to_user(buf + result, runtime->buffer + runtime->appl_ptr, count1)) {
>>>-				spin_unlock_irqrestore(&runtime->lock, flags);
>>> 				return result > 0 ? result : -EFAULT;
>>> 			}
>>>+			spin_lock_irqsave(&runtime->lock, flags);
>>> 		}
>>> 		runtime->appl_ptr += count1;
>>> 		runtime->appl_ptr %= runtime->buffer_size;
>>>@@ -1133,10 +1134,13 @@
>>> 		if (kernel) {
>>> 			memcpy(runtime->buffer + runtime->appl_ptr, buf, count1);
>>> 		} else {
>>>+			spin_unlock_irqrestore(&runtime->lock, flags);
>>> 			if (copy_from_user(runtime->buffer + runtime->appl_ptr, buf, count1)) {
>>>+				spin_lock_irqsave(&runtime->lock, flags);
>>> 				result = result > 0 ? result : -EFAULT;
>>> 				goto __end;
>>> 			}
>>>+			spin_lock_irqsave(&runtime->lock, flags);
>>> 		}
>>> 		runtime->appl_ptr += count1;
>>> 		runtime->appl_ptr %= runtime->buffer_size;
>>
>>
>>With this patch you break write and read atomicity, you should:
>>
>>1) read and save appl_ptr
>>2) update appl_ptr and avail
>>3) leave critical area
>>4) copy from/to user using saved appl_ptr
> 
> 
> But hardware can overwrite data (read) or send data which are not yet 
> available (write) when we are not in the spinlock context and data are not 
> copied.

I see.

> If we need atomic read() and write() we must probably add a semaphore.

Too heavyweight, I propose:

1) size_t ptr = appl_ptr_next
2) update appl_ptr_next etc.
3) pending++
4) spin_unlock
5) copy from/to user
6) spin_lock
7) if (--pending) appl_ptr = appl_ptr_next

-- 
Abramo Bagnara                       mailto:abramo.bagnara@tin.it

Opera Unica                          Phone: +39.0546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn