[alsa-devel] [PATCH 2/2] ALSA: oxfw: discontinue MIDI substream for scs1x at transaction failure

[Takashi Iwai]

On Fri, 19 Feb 2016 17:04:14 +0100,
Takashi Iwai wrote:
> 
> On Fri, 19 Feb 2016 16:51:55 +0100,
> Takashi Sakamoto wrote:
> > 
> > On Feb 19 2016 18:46, Takashi Iwai wrote:
> > > On Fri, 19 Feb 2016 10:23:36 +0100,
> > > Takashi Sakamoto wrote:
> > >>
> > >> Hi,
> > >>
> > >> On Feb 19 2016 17:19, Takashi Iwai wrote:
> > >>> On Fri, 19 Feb 2016 01:55:50 +0100,
> > >>> Takashi Sakamoto wrote:
> > >>>>
> > >>>> With a previous commit, ALSA oxfw driver retries transferring MIDI
> > >>>> messages at transaction failure for scs1x. On the other hand, there're
> > >>>> fatal transaction error. Then, MIDI messages never reach to the unit
> > >>>> anymore. In this case, MIDI substream should be discontinued.
> > >>>>
> > >>>> This commit stops MIDI transferring after the fatal error occurs.
> > >>>> Unfortunately, unlike ALSA PCM functionality, ALSA rawmidi core has no
> > >>>> feature to discontinue MIDI substream in kernel side, thus this commit
> > >>>> just stops MIDI transferring without notifying it to userspace.
> > >>>
> > >>> It's fine to take this, and I would take it as is for now.
> > >>
> > >> OK.
> > >>
> > >>> But we can extend the rawmidi somehow to deal with such an error, too.
> > >>> Maybe just having "error" flag in the rawmidi runtime and adding a
> > >>> helper function to set the error and stop the stream should work
> > >>> easily.
> > >>
> > >> You forgot ALSA sequencer.
> > > 
> > > This is a different layer.  The sequencer stuff may check the error
> > > and handle more gracefully than as of now, but it's a different story
> > > from the extension of rawmidi itself.
> > > 
> > > What you're working on is the rawmidi, and what it can give is just
> > > returning an error at the right moment.
> > 
> > I don't think so. I think the work is heavier than your expectation.
> > 
> > I pushed 'rawmidi-epipe' branch to my repository.
> > https://github.com/takaswie/sound/tree/rawmidi-epipe
> > 
> > You can see four commits just to show my concerns.
> > (They're not tested yet. Don't use them for actual work ;)
> > 41f499b ALSA: rawmidi: add a helper to set runtime error
> > 4164edb ALSA: rawmidi: handle EPIPE
> > 5e7348ed ALSA: seq: handle EPIPE for rawmidi input
> > 58d9008 ALSA: seq: handle EPIPE for rawmidi output
> > 
> > In the top-most commit, you can see userspace applications need to
> > close/open character devices to recover from EPIPE state of rawmidi
> > substream runtime.
> 
> Yes, this doesn't change from the current situation.
> 
> Remember that rawmidi may return an error already in the current
> implementation.  The new feature would be just to add the explicit
> trigger of the error, instead of implicit runtime->avail check.

BTW, you'd need to put more error checks.  There are other loops in
snd_rawmidi_read() and snd_rawmidi_write() before
snd_rawmidi_kernel_read1() and snd_rawmidi_kernel_write1() is reached.
The check is needed there, too.


Takashi