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

[Takashi Sakamoto]

On 2016年02月20日 01:11, Takashi Iwai wrote:
> 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.

OK, thanks. I'll check it later and it's time to go to bed.


zzz

Takashi Sakamoto