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

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

Abramo Bagnara ha scritto:
> 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

Using ptr of course

> 6) spin_lock
> 7) if (--pending) appl_ptr = appl_ptr_next

Sorry, this should be:
7) if (--pending == 0) 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