echoaudio.c, Revision 1.6

echoaudio.c, Revision 1.6

[Giuliano Pochini]

I'm a bit confused about the latest changes to pci/echoaudio/echoaudio.c

The CVS interface says: "Use spin_lock_irqsave() instead of spin_lock() in
places where we could be interrupted by another hardware interrupt that
could call the rawmidi trigger callback that could try to take the same
lock."

But in snd_echo_interrupt() we are running in hard irq context, so we know
irqs are already disabled. We should change midi.c:snd_echo_midi_*_trigger()
instead, or am I missing something ?


Then: "Additionally, remove locking code that is no longer needed now that
the trigger callback is no longer called recursively from the rawmidi
"event" handler."

Your patch does not remove anything. It only replaces spin_lock() in
pcm_trigger() which is a stupid bug of mine, sorry.



--
Giuliano.


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: echoaudio.c, Revision 1.6

[Clemens Ladisch]

Giuliano Pochini wrote:
> I'm a bit confused about the latest changes to pci/echoaudio/echoaudio.c
>
> The CVS interface says: "Use spin_lock_irqsave() instead of spin_lock() in
> places where we could be interrupted by another hardware interrupt that
> could call the rawmidi trigger callback that could try to take the same
> lock."
>
> But in snd_echo_interrupt() we are running in hard irq context, so we know
> irqs are already disabled.

Only the sound card interrupt itself is disabled.  It is architecture-
specific whether other interrupts are disabled.

> Then: "Additionally, remove locking code that is no longer needed now that
> the trigger callback is no longer called recursively from the rawmidi
> "event" handler."
>
> Your patch does not remove anything.

It removed code in some other files.

> It only replaces spin_lock() in pcm_trigger() which is a stupid
> bug of mine, sorry.

I think this wasn't a bug because the trigger callback is already
called with interrupts disabled, AFAICS.  I'm planning to revert most
of these locking changes when I've proven that it's safe.


Regards,
Clemens



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: echoaudio.c, Revision 1.6

[Takashi Iwai]

At Mon, 14 Mar 2005 09:19:28 +0100 (MET),
Clemens Ladisch wrote:
> 
> Giuliano Pochini wrote:
> > I'm a bit confused about the latest changes to pci/echoaudio/echoaudio.c
> >
> > The CVS interface says: "Use spin_lock_irqsave() instead of spin_lock() in
> > places where we could be interrupted by another hardware interrupt that
> > could call the rawmidi trigger callback that could try to take the same
> > lock."
> >
> > But in snd_echo_interrupt() we are running in hard irq context, so we know
> > irqs are already disabled.
> 
> Only the sound card interrupt itself is disabled.  It is architecture-
> specific whether other interrupts are disabled.
> 
> > Then: "Additionally, remove locking code that is no longer needed now that
> > the trigger callback is no longer called recursively from the rawmidi
> > "event" handler."
> >
> > Your patch does not remove anything.
> 
> It removed code in some other files.
> 
> > It only replaces spin_lock() in pcm_trigger() which is a stupid
> > bug of mine, sorry.
> 
> I think this wasn't a bug because the trigger callback is already
> called with interrupts disabled, AFAICS.  I'm planning to revert most
> of these locking changes when I've proven that it's safe.

I fixed snd_pcm_suspend*() functions to do in irq disabled.
So, all calls of trigger and pointer callbacks are atomic now.


Takashi


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: echoaudio.c, Revision 1.6

[Giuliano Pochini]

On Mon, 14 Mar 2005, Clemens Ladisch wrote:

> Giuliano Pochini wrote:
> > I'm a bit confused about the latest changes to pci/echoaudio/echoaudio.c
> >
> > The CVS interface says: "Use spin_lock_irqsave() instead of spin_lock() in
> > places where we could be interrupted by another hardware interrupt that
> > could call the rawmidi trigger callback that could try to take the same
> > lock."
> >
> > But in snd_echo_interrupt() we are running in hard irq context, so we know
> > irqs are already disabled.
>
> Only the sound card interrupt itself is disabled.  It is architecture-
> specific whether other interrupts are disabled.

Yes, but chip_t->mylock is per device, it's not the same lock even if I
have 2 identical cards. And the driver calls snd_pcm_period_elapsed() and
snd_rawmidi_receive() when the lock is not held. I really can't see why
the irq handler requires _irqsave spinlocks.


> > Then: "Additionally, remove locking code that is no longer needed now that
> > the trigger callback is no longer called recursively from the rawmidi
> > "event" handler."
> >
> > Your patch does not remove anything.
>
> It removed code in some other files.

I diff'd all the files and only echoaudio.c was changed.


--
Giuliano.


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: echoaudio.c, Revision 1.6

[Clemens Ladisch]

Giuliano Pochini wrote:
>
> On Mon, 14 Mar 2005, Clemens Ladisch wrote:
>
> > Giuliano Pochini wrote:
> >
> > > The CVS interface says: "Use spin_lock_irqsave() instead of spin_lock() in
> > > places where we could be interrupted by another hardware interrupt that
> > > could call the rawmidi trigger callback that could try to take the same
> > > lock."
> > >
> > > But in snd_echo_interrupt() we are running in hard irq context, so we know
> > > irqs are already disabled.
> >
> > Only the sound card interrupt itself is disabled.  It is architecture-
> > specific whether other interrupts are disabled.
>
> Yes, but chip_t->mylock is per device, it's not the same lock even if I
> have 2 identical cards. And the driver calls snd_pcm_period_elapsed() and
> snd_rawmidi_receive() when the lock is not held. I really can't see why
> the irq handler requires _irqsave spinlocks.

When MIDI files are played, some timer interrupt (which can be the
timer of another sound card) can cause some MIDI data to be written to
the MIDI port.

> > It removed code in some other files.
>
> I diff'd all the files and only echoaudio.c was changed.

Revision 1.33 of alsa-kernel/drivers/mpu401/mpu401_uart.c is one of
these changes.


Regards,
Clemens



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: echoaudio.c, Revision 1.6

[Giuliano Pochini]

On Wed, 16 Mar 2005 09:20:49 +0100 (MET)
Clemens Ladisch <clemens@ladisch.de> wrote:

> > Yes, but chip_t->mylock is per device, it's not the same lock even if I
> > have 2 identical cards. And the driver calls snd_pcm_period_elapsed() and
> > snd_rawmidi_receive() when the lock is not held. I really can't see why
> > the irq handler requires _irqsave spinlocks.
>
> When MIDI files are played, some timer interrupt (which can be the
> timer of another sound card) can cause some MIDI data to be written to
> the MIDI port.

Which causes the rawmidi (in my case) callbacks to be called from irq
context, so they must use _irqsave locks. But in the irq handler what's the
difference between

spin_lock()
card_io
spin_unlock()
snd_rawmidi_receive()
return

and

spin_lock_irqsave()
card_io
spin_unlock_irqrestore()
snd_rawmidi_receive()
return

? IMHO they're the same from the viewpoint of snd_rawmidi_receive().


--
Giuliano.


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: echoaudio.c, Revision 1.6

[Clemens Ladisch]

Giuliano Pochini wrote:
> Clemens Ladisch <clemens@ladisch.de> wrote:
> > When MIDI files are played, some timer interrupt (which can be the
> > timer of another sound card) can cause some MIDI data to be written to
> > the MIDI port.
>
> Which causes the rawmidi (in my case) callbacks to be called from irq
> context, so they must use _irqsave locks.

Not anymore, I just moved the callbacks call into tasklet.

> But in the irq handler what's the difference between
>
> spin_lock()
> card_io
> spin_unlock()
> snd_rawmidi_receive()
> return
>
> and
>
> spin_lock_irqsave()
> card_io
> spin_unlock_irqrestore()
> snd_rawmidi_receive()
> return
>
> ? IMHO they're the same from the viewpoint of snd_rawmidi_receive().

Indeed they are.  In the old version, snd_rawmidi_receive() would
recursively call the trigger callback (which could try to take the
same lock), but that call was moved to a tasklet, too.


Regards,
Clemens



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click