[alsa-devel] [PATCH 0/2] ALSA: oxfw: implement retries for scs1x at transaction failure
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(-)
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);
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);
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
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
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
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
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
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
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
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
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
participants (2)
-
Takashi Iwai -
Takashi Sakamoto