[PATCH 0/2] ALSA: oxfw: implement retries for scs1x at transaction failure

[PATCH 0/2] ALSA: oxfw: implement retries for scs1x at transaction failure

[Takashi Sakamoto]

Hi,

Current ALSA oxfw driver has a TODO to retry IEEE 1394 asynchronous
transactions when transaction failure occurs. This patchset is for it.
Additionally, this patchset classifies reason of the failure and stop
the retries to prevent from endless loop of scheduling work.

Regards

Takashi Sakamoto (2):
  ALSA: oxfw: retry MIDI transferring for scs1x at transaction failure
  ALSA: oxfw: discontinue MIDI substream for scs1x at transaction
    failure

 sound/firewire/oxfw/oxfw-scs1x.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

-- 
2.5.0

[PATCH 1/2] ALSA: oxfw: retry MIDI transferring for scs1x at transaction failure

[Takashi Sakamoto]

Currently, ALSA oxfw driver has a TODO to retry MIDI transferring
at transaction failure.

This commit achieves it. Current implementation uses snd_rawmidi_transmit()
to transfer messages, thus the target MIDI messages are not in buffer when
transaction failure is detected. So we cannot use a pair of
snd_rawmidi_transmit_peek() and snd_ramwidi_transmit_ack(). But the
messages are in scs1x specific structure and the data is available for
retries.

This commit adds a member to the structure for the length of messages and
uses the value again at retries.
---
 sound/firewire/oxfw/oxfw-scs1x.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/oxfw/oxfw-scs1x.c b/sound/firewire/oxfw/oxfw-scs1x.c
index f7ac124..72446ac 100644
--- a/sound/firewire/oxfw/oxfw-scs1x.c
+++ b/sound/firewire/oxfw/oxfw-scs1x.c
@@ -31,6 +31,7 @@ struct fw_scs1x {
 	u8 buffer[HSS1394_MAX_PACKET_SIZE];
 	bool transaction_running;
 	struct fw_transaction transaction;
+	unsigned int transaction_bytes;
 	struct fw_device *fw_dev;
 };
 
@@ -125,8 +126,8 @@ static void scs_write_callback(struct fw_card *card, int rcode,
 {
 	struct fw_scs1x *scs = callback_data;
 
-	if (rcode == RCODE_GENERATION)
-		;	/* TODO: retry this packet */
+	if (rcode != RCODE_GENERATION)
+		scs->transaction_bytes = 0;
 
 	scs->transaction_running = false;
 	schedule_work(&scs->work);
@@ -183,6 +184,9 @@ static void scs_output_work(struct work_struct *work)
 		return;
 	}
 
+	if (scs->transaction_bytes > 0)
+		goto retry;
+
 	i = scs->output_bytes;
 	for (;;) {
 		if (snd_rawmidi_transmit(stream, &byte, 1) != 1) {
@@ -253,13 +257,16 @@ static void scs_output_work(struct work_struct *work)
 	scs->output_bytes = 1;
 	scs->output_escaped = false;
 
+	scs->transaction_bytes = i;
+retry:
 	scs->transaction_running = true;
 	generation = scs->fw_dev->generation;
 	smp_rmb(); /* node_id vs. generation */
 	fw_send_request(scs->fw_dev->card, &scs->transaction,
 			TCODE_WRITE_BLOCK_REQUEST, scs->fw_dev->node_id,
 			generation, scs->fw_dev->max_speed, HSS1394_ADDRESS,
-			scs->buffer, i, scs_write_callback, scs);
+			scs->buffer, scs->transaction_bytes,
+			scs_write_callback, scs);
 }
 
 static int midi_capture_open(struct snd_rawmidi_substream *stream)
@@ -309,6 +316,7 @@ static void midi_playback_trigger(struct snd_rawmidi_substream *stream, int up)
 		scs->output_bytes = 1;
 		scs->output_escaped = false;
 		scs->output_idle = false;
+		scs->transaction_bytes = 0;
 
 		ACCESS_ONCE(scs->output) = stream;
 		schedule_work(&scs->work);
-- 
2.5.0

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

[Takashi Sakamoto]

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.
---
 sound/firewire/oxfw/oxfw-scs1x.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/oxfw/oxfw-scs1x.c b/sound/firewire/oxfw/oxfw-scs1x.c
index 72446ac..f897c98 100644
--- a/sound/firewire/oxfw/oxfw-scs1x.c
+++ b/sound/firewire/oxfw/oxfw-scs1x.c
@@ -32,6 +32,7 @@ struct fw_scs1x {
 	bool transaction_running;
 	struct fw_transaction transaction;
 	unsigned int transaction_bytes;
+	bool error;
 	struct fw_device *fw_dev;
 };
 
@@ -126,8 +127,13 @@ static void scs_write_callback(struct fw_card *card, int rcode,
 {
 	struct fw_scs1x *scs = callback_data;
 
-	if (rcode != RCODE_GENERATION)
-		scs->transaction_bytes = 0;
+	if (!rcode_is_permanent_error(rcode)) {
+		/* Don't retry for this data. */
+		if (rcode == RCODE_COMPLETE)
+			scs->transaction_bytes = 0;
+	} else {
+		scs->error = true;
+	}
 
 	scs->transaction_running = false;
 	schedule_work(&scs->work);
@@ -178,7 +184,7 @@ static void scs_output_work(struct work_struct *work)
 		return;
 
 	stream = ACCESS_ONCE(scs->output);
-	if (!stream) {
+	if (!stream || scs->error) {
 		scs->output_idle = true;
 		wake_up(&scs->idle_wait);
 		return;
@@ -317,6 +323,7 @@ static void midi_playback_trigger(struct snd_rawmidi_substream *stream, int up)
 		scs->output_escaped = false;
 		scs->output_idle = false;
 		scs->transaction_bytes = 0;
+		scs->error = false;
 
 		ACCESS_ONCE(scs->output) = stream;
 		schedule_work(&scs->work);
-- 
2.5.0

Re: [PATCH 0/2] ALSA: oxfw: implement retries for scs1x at transaction failure

[Takashi Iwai]

On Fri, 19 Feb 2016 01:55:48 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> Current ALSA oxfw driver has a TODO to retry IEEE 1394 asynchronous
> transactions when transaction failure occurs. This patchset is for it.
> Additionally, this patchset classifies reason of the failure and stop
> the retries to prevent from endless loop of scheduling work.
> 
> Regards
> 
> Takashi Sakamoto (2):
>   ALSA: oxfw: retry MIDI transferring for scs1x at transaction failure
>   ALSA: oxfw: discontinue MIDI substream for scs1x at transaction
>     failure

Your sign-off is missing in both patches.  Please give it explicitly.


Takashi

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

[Takashi Iwai]

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.
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.


thanks,

Takashi

Re: [PATCH 0/2] ALSA: oxfw: implement retries for scs1x at transaction failure

[Takashi Sakamoto]

On Feb 19 2016 17:17, Takashi Iwai wrote:
> On Fri, 19 Feb 2016 01:55:48 +0100,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> Current ALSA oxfw driver has a TODO to retry IEEE 1394 asynchronous
>> transactions when transaction failure occurs. This patchset is for it.
>> Additionally, this patchset classifies reason of the failure and stop
>> the retries to prevent from endless loop of scheduling work.
>>
>> Regards
>>
>> Takashi Sakamoto (2):
>>    ALSA: oxfw: retry MIDI transferring for scs1x at transaction failure
>>    ALSA: oxfw: discontinue MIDI substream for scs1x at transaction
>>      failure
>
> Your sign-off is missing in both patches.  Please give it explicitly.

Oops. I had the oversight. I usually execute checkpatch.pl before 
posting, but I might have forgot even it in this morning, sorry...


Takashi Sakamoto
o-takashi@sakamocchi.jp

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

[Takashi Sakamoto]

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.


I had considered about it when working for snd-firewire-digi00x (Mar 
2015), and realised that it may take much time. At least, we work for:
  - ALSA rawmidi core
  - ALSA rawmidi interface for userspace

Additionally, users mostly use rawmidi devices via ALSA sequencer:
  - ALSA sequencer core
  - ALSA sequencer interface for userspace

Furthermore, regression test is required for:
  - ALSA drivers with MIDI functionality
  - ALSA rawmidi/sequencer applications in userspace

It's not 2000s. I guess that little developers are attracted for this 
work. In my option, the work is really cost-expensive than actual merit. 
So currently I'm negative for the idea.


Regards

Takashi Sakamoto

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

[Takashi Iwai]

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 had considered about it when working for snd-firewire-digi00x (Mar 
> 2015), and realised that it may take much time. At least, we work for:
>   - ALSA rawmidi core
>   - ALSA rawmidi interface for userspace

We don't need to extend rawmidi user-space API so much.  Again, what
needed is to return an error at read/write, probably -EPIPE, right
after the driver got a hardware error.  For that, just adding
runtime->error flag and checking it appropriate should suffice (in
theory).  The read/write in rawmidi is pretty simple.  And we don't
have to reset the flag until the stream is reopened.  There is no
setup/preparation phase for rawmidi unlike PCM.

> Additionally, users mostly use rawmidi devices via ALSA sequencer:
>   - ALSA sequencer core
>   - ALSA sequencer interface for userspace
> 
> Furthermore, regression test is required for:
>   - ALSA drivers with MIDI functionality
>   - ALSA rawmidi/sequencer applications in userspace
> 
> It's not 2000s. I guess that little developers are attracted for this 
> work. In my option, the work is really cost-expensive than actual merit. 
> So currently I'm negative for the idea.

A regression test is needed of course, but a simple error check
can't regress except for the driver that really supports it.


Takashi

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

[Takashi Sakamoto]

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. Furthermore, this also means that kernel
implemenation such as ALSA sequencer also need to destroy/create rawmidi
substream.

The main reason of them is ALSA rawmidi core has no functionality for
recovery feature. In ALSA PCM core, 'struct snd_pcm_ops.prepare' has a
role for it.

>> I had considered about it when working for snd-firewire-digi00x (Mar 
>> 2015), and realised that it may take much time. At least, we work for:
>>   - ALSA rawmidi core
>>   - ALSA rawmidi interface for userspace
> 
> We don't need to extend rawmidi user-space API so much.  Again, what
> needed is to return an error at read/write, probably -EPIPE, right
> after the driver got a hardware error.  For that, just adding
> runtime->error flag and checking it appropriate should suffice (in
> theory).  The read/write in rawmidi is pretty simple.  And we don't
> have to reset the flag until the stream is reopened.  There is no
> setup/preparation phase for rawmidi unlike PCM.
> 
>> Additionally, users mostly use rawmidi devices via ALSA sequencer:
>>   - ALSA sequencer core
>>   - ALSA sequencer interface for userspace
>>
>> Furthermore, regression test is required for:
>>   - ALSA drivers with MIDI functionality
>>   - ALSA rawmidi/sequencer applications in userspace
>>
>> It's not 2000s. I guess that little developers are attracted for this 
>> work. In my option, the work is really cost-expensive than actual merit. 
>> So currently I'm negative for the idea.
> 
> A regression test is needed of course, but a simple error check
> can't regress except for the driver that really supports it.


Regards

Takashi Sakamoto

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

[Takashi Iwai]

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.

> Furthermore, this also means that kernel
> implemenation such as ALSA sequencer also need to destroy/create rawmidi
> substream.

Only if sequencer layer matters.  Currently it just ignores.

> The main reason of them is ALSA rawmidi core has no functionality for
> recovery feature. In ALSA PCM core, 'struct snd_pcm_ops.prepare' has a
> role for it.

It'll never be, I guess.  The reopen is the simplest solution for
devices like rawmidi, and we have no big reason to make things
complicated.


Takashi

Re: [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

Re: [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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Sakamoto]

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, no MIDI messages reach to the unit anymore.
In this case, MIDI substream should be terminated.

This commit stops MIDI transmission after the fatal error occurs.
Unfortunately, unlike ALSA PCM functionality, ALSA rawmidi core has no
feature to discontinue MIDI substream runtime in kernel side, thus this
commit just stops MIDI transmission without notifying it to userspace.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/oxfw/oxfw-scs1x.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/oxfw/oxfw-scs1x.c b/sound/firewire/oxfw/oxfw-scs1x.c
index 72446ac..f897c98 100644
--- a/sound/firewire/oxfw/oxfw-scs1x.c
+++ b/sound/firewire/oxfw/oxfw-scs1x.c
@@ -32,6 +32,7 @@ struct fw_scs1x {
 	bool transaction_running;
 	struct fw_transaction transaction;
 	unsigned int transaction_bytes;
+	bool error;
 	struct fw_device *fw_dev;
 };
 
@@ -126,8 +127,13 @@ static void scs_write_callback(struct fw_card *card, int rcode,
 {
 	struct fw_scs1x *scs = callback_data;
 
-	if (rcode != RCODE_GENERATION)
-		scs->transaction_bytes = 0;
+	if (!rcode_is_permanent_error(rcode)) {
+		/* Don't retry for this data. */
+		if (rcode == RCODE_COMPLETE)
+			scs->transaction_bytes = 0;
+	} else {
+		scs->error = true;
+	}
 
 	scs->transaction_running = false;
 	schedule_work(&scs->work);
@@ -178,7 +184,7 @@ static void scs_output_work(struct work_struct *work)
 		return;
 
 	stream = ACCESS_ONCE(scs->output);
-	if (!stream) {
+	if (!stream || scs->error) {
 		scs->output_idle = true;
 		wake_up(&scs->idle_wait);
 		return;
@@ -317,6 +323,7 @@ static void midi_playback_trigger(struct snd_rawmidi_substream *stream, int up)
 		scs->output_escaped = false;
 		scs->output_idle = false;
 		scs->transaction_bytes = 0;
+		scs->error = false;
 
 		ACCESS_ONCE(scs->output) = stream;
 		schedule_work(&scs->work);
-- 
2.5.0