* Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 [not found] <E1AnKYI-0000sI-Hg@sc8-pr-cvs1.sourceforge.net> @ 2004-02-01 17:04 ` Abramo Bagnara 2004-02-02 8:41 ` Jaroslav Kysela 2004-02-02 11:13 ` Takashi Iwai 0 siblings, 2 replies; 5+ messages in thread From: Abramo Bagnara @ 2004-02-01 17:04 UTC (permalink / raw) To: Jaroslav Kysela, alsa-devel 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 5) reenter critical area I've just checked and a similar mistake has been done too for PCM (and other places?). -- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 2004-02-01 17:04 ` [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 Abramo Bagnara @ 2004-02-02 8:41 ` Jaroslav Kysela 2004-02-02 10:12 ` Abramo Bagnara 2004-02-02 11:13 ` Takashi Iwai 1 sibling, 1 reply; 5+ messages in thread From: Jaroslav Kysela @ 2004-02-02 8:41 UTC (permalink / raw) To: Abramo Bagnara; +Cc: alsa-devel 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. If we need atomic read() and write() we must probably add a semaphore. > 5) reenter critical area > > I've just checked and a similar mistake has been done too for PCM (and > other places?). Yes, but the question is, in which case is atomicity necessary. So far, I see the problem only for the O_APPEND mode (rawmidi). Jaroslav ----- Jaroslav Kysela <perex@suse.cz> Linux Kernel Sound Maintainer ALSA Project, SuSE Labs ------------------------------------------------------- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 2004-02-02 8:41 ` Jaroslav Kysela @ 2004-02-02 10:12 ` Abramo Bagnara 2004-02-02 10:19 ` Abramo Bagnara 0 siblings, 1 reply; 5+ messages in thread From: Abramo Bagnara @ 2004-02-02 10:12 UTC (permalink / raw) To: Jaroslav Kysela; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 2004-02-02 10:12 ` Abramo Bagnara @ 2004-02-02 10:19 ` Abramo Bagnara 0 siblings, 0 replies; 5+ messages in thread From: Abramo Bagnara @ 2004-02-02 10:19 UTC (permalink / raw) Cc: Jaroslav Kysela, alsa-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 2004-02-01 17:04 ` [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 Abramo Bagnara 2004-02-02 8:41 ` Jaroslav Kysela @ 2004-02-02 11:13 ` Takashi Iwai 1 sibling, 0 replies; 5+ messages in thread From: Takashi Iwai @ 2004-02-02 11:13 UTC (permalink / raw) To: Abramo Bagnara; +Cc: Jaroslav Kysela, alsa-devel At Sun, 01 Feb 2004 18:04:14 +0100, Abramo Bagnara wrote: (snip) > 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 > 5) reenter critical area > > I've just checked and a similar mistake has been done too for PCM (and > other places?). or simply use semaphore instead of spinlock? (kernel=1 can be in the interrupt context, so it'd better to down/up semaphore in snd_rawmidi_read(), write(), etc.) Takashi ------------------------------------------------------- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2004-02-02 11:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <E1AnKYI-0000sI-Hg@sc8-pr-cvs1.sourceforge.net>
2004-02-01 17:04 ` [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41 Abramo Bagnara
2004-02-02 8:41 ` Jaroslav Kysela
2004-02-02 10:12 ` Abramo Bagnara
2004-02-02 10:19 ` Abramo Bagnara
2004-02-02 11:13 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.