[alsa-devel] [PATCH 0/5 v2] ALSA: firewire-tascam: fix sending the same MIDI bytes in two transactions
Hi,
This patchset is revised version of my previous post: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/099123.htm...
The main purpose of this patchset is to fix an issue that a transaction can include MIDI bytes in the next message. The last patch solves this issue, and the first 4 patches are for better shape of code including a small fixes.
Changes: * add buffer alias * refine local variables
Takashi Sakamoto (5): ALSA: firewire-tascam: remove buffer initialization in driver side ALSA: firewire-tascam: change type of valiables according to function prototype ALSA: firewire-tascam: use better name for local variables to describe their intension ALSA: firewire-tascam: fix loop condition with some readable variables ALSA: firewire-tascam: clear extra MIDI byte in an asynchronous transaction
sound/firewire/tascam/tascam-transaction.c | 73 +++++++++++++++++------------- 1 file changed, 41 insertions(+), 32 deletions(-)
The given buffer to callback function is cleared in caller side.
This commit removes buffer initialization in callee side.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index d4f64ae..370f932 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -67,8 +67,6 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) u8 status; int consume;
- buf[0] = buf[1] = buf[2] = buf[3] = 0x00; - len = snd_rawmidi_transmit_peek(substream, buf + 1, 3); if (len == 0) return 0;
In the callback function of asynchronous MIDI port, some local variables are declared 'unsigned int', while they're assigned to int value of return from snd_rawmidi_transmit_peek().
This commit fixes the type.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 370f932..7c80684 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -62,10 +62,8 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) { struct snd_tscm *tscm = substream->rmidi->private_data; unsigned int port = substream->number; - unsigned int len; - unsigned int i; + int i, len, consume; u8 status; - int consume;
len = snd_rawmidi_transmit_peek(substream, buf + 1, 3); if (len == 0)
In the callback function of asynchronous MIDI port, the intension of some local variables are not clear.
This commit improves them. The 'len' variable is used to calculate the number of MIDI bytes including in the transaction. The 'consume' variable is used to return the actual number of consumed bytes in ALSA MIDI buffer.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 7c80684..ea88655 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -65,14 +65,14 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) int i, len, consume; u8 status;
- len = snd_rawmidi_transmit_peek(substream, buf + 1, 3); - if (len == 0) + consume = snd_rawmidi_transmit_peek(substream, buf + 1, 3); + if (consume == 0) return 0;
/* On exclusive message. */ if (tscm->on_sysex[port]) { /* Seek the end of exclusives. */ - for (i = 1; i < 4 || i < len; ++i) { + for (i = 1; i < 4 || i < consume; ++i) { if (buf[i] == 0xf7) { tscm->on_sysex[port] = false; break; @@ -81,14 +81,14 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
/* At the end of exclusive message, use label 0x07. */ if (!tscm->on_sysex[port]) { - len = i; + consume = i; buf[0] = (port << 4) | 0x07; /* During exclusive message, use label 0x04. */ - } else if (len == 3) { + } else if (consume == 3) { buf[0] = (port << 4) | 0x04; /* We need to fill whole 3 bytes. Go to next change. */ } else { - len = 0; + consume = 0; } } else { /* The beginning of exclusives. */ @@ -104,8 +104,8 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) status = buf[1];
/* Calculate consume bytes. */ - consume = calculate_message_bytes(status); - if (consume <= 0) + len = calculate_message_bytes(status); + if (len <= 0) return 0;
/* On running-status. */ @@ -119,16 +119,16 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) }
/* Confirm length. */ - if (len < consume) + if (consume < len) return 0; - if (len > consume) - len = consume; + if (consume > len) + consume = len; }
buf[0] = (port << 4) | (buf[1] >> 4); }
- return len; + return consume; }
static void handle_midi_tx(struct fw_card *card, struct fw_request *request,
In transactions for MIDI messages, the first byte is used for label and the rest is for MIDI bytes. In current code, these are handled correctly, while there's a small mistake for loop condition to include meaningless statement.
This commit adds two local variables for them and improve the loop condition.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 35 +++++++++++++++++------------- 1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index ea88655..99098aa 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -63,17 +63,22 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) struct snd_tscm *tscm = substream->rmidi->private_data; unsigned int port = substream->number; int i, len, consume; + u8 *label, *msg; u8 status;
- consume = snd_rawmidi_transmit_peek(substream, buf + 1, 3); + /* The first byte is used for label, the rest for MIDI bytes. */ + label = buf; + msg = buf + 1; + + consume = snd_rawmidi_transmit_peek(substream, msg, 3); if (consume == 0) return 0;
/* On exclusive message. */ if (tscm->on_sysex[port]) { /* Seek the end of exclusives. */ - for (i = 1; i < 4 || i < consume; ++i) { - if (buf[i] == 0xf7) { + for (i = 0; i < consume; ++i) { + if (msg[i] == 0xf7) { tscm->on_sysex[port] = false; break; } @@ -81,27 +86,27 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
/* At the end of exclusive message, use label 0x07. */ if (!tscm->on_sysex[port]) { - consume = i; - buf[0] = (port << 4) | 0x07; + consume = i + 1; + *label = (port << 4) | 0x07; /* During exclusive message, use label 0x04. */ } else if (consume == 3) { - buf[0] = (port << 4) | 0x04; + *label = (port << 4) | 0x04; /* We need to fill whole 3 bytes. Go to next change. */ } else { consume = 0; } } else { /* The beginning of exclusives. */ - if (buf[1] == 0xf0) { + if (msg[0] == 0xf0) { /* Transfer it in next chance in another condition. */ tscm->on_sysex[port] = true; return 0; } else { /* On running-status. */ - if ((buf[1] & 0x80) != 0x80) + if ((msg[0] & 0x80) != 0x80) status = tscm->running_status[port]; else - status = buf[1]; + status = msg[0];
/* Calculate consume bytes. */ len = calculate_message_bytes(status); @@ -109,13 +114,13 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) return 0;
/* On running-status. */ - if ((buf[1] & 0x80) != 0x80) { - buf[3] = buf[2]; - buf[2] = buf[1]; - buf[1] = tscm->running_status[port]; + if ((msg[0] & 0x80) != 0x80) { + msg[2] = msg[1]; + msg[1] = msg[0]; + msg[0] = tscm->running_status[port]; consume--; } else { - tscm->running_status[port] = buf[1]; + tscm->running_status[port] = msg[0]; }
/* Confirm length. */ @@ -125,7 +130,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) consume = len; }
- buf[0] = (port << 4) | (buf[1] >> 4); + *label = (port << 4) | (msg[0] >> 4); }
return consume;
When MIDI buffer stores two or more MIDI messages, TASCAM driver transfers asynchronous transactions including one MIDI message and extra bytes from second MIDI message.
This commit fixes this bug by clearing needless bytes in the buffer. The consumed bytes are already calculated correctly, thus the sequence of transactions is already correct.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-transaction.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c index 99098aa..904ce03 100644 --- a/sound/firewire/tascam/tascam-transaction.c +++ b/sound/firewire/tascam/tascam-transaction.c @@ -93,8 +93,10 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf) *label = (port << 4) | 0x04; /* We need to fill whole 3 bytes. Go to next change. */ } else { - consume = 0; + return 0; } + + len = consume; } else { /* The beginning of exclusives. */ if (msg[0] == 0xf0) { @@ -115,24 +117,30 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
/* On running-status. */ if ((msg[0] & 0x80) != 0x80) { + /* Enough MIDI bytes were not retrieved. */ + if (consume < len - 1) + return 0; + consume = len - 1; + msg[2] = msg[1]; msg[1] = msg[0]; msg[0] = tscm->running_status[port]; - consume--; } else { + /* Enough MIDI bytes were not retrieved. */ + if (consume < len) + return 0; + consume = len; + tscm->running_status[port] = msg[0]; } - - /* Confirm length. */ - if (consume < len) - return 0; - if (consume > len) - consume = len; }
*label = (port << 4) | (msg[0] >> 4); }
+ if (len > 0 && len < 3) + memset(msg + len, 0, 3 - len); + return consume; }
On Tue, 20 Oct 2015 16:46:54 +0200, Takashi Sakamoto wrote:
Hi,
This patchset is revised version of my previous post: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/099123.htm...
The main purpose of this patchset is to fix an issue that a transaction can include MIDI bytes in the next message. The last patch solves this issue, and the first 4 patches are for better shape of code including a small fixes.
Changes:
- add buffer alias
- refine local variables
Applies all patches now. Thanks.
Takashi
Takashi Sakamoto (5): ALSA: firewire-tascam: remove buffer initialization in driver side ALSA: firewire-tascam: change type of valiables according to function prototype ALSA: firewire-tascam: use better name for local variables to describe their intension ALSA: firewire-tascam: fix loop condition with some readable variables ALSA: firewire-tascam: clear extra MIDI byte in an asynchronous transaction
sound/firewire/tascam/tascam-transaction.c | 73 +++++++++++++++++------------- 1 file changed, 41 insertions(+), 32 deletions(-)
-- 2.1.4
On 2015年10月21日 03:11, Takashi Iwai wrote:
On Tue, 20 Oct 2015 16:46:54 +0200, Takashi Sakamoto wrote:
Hi,
This patchset is revised version of my previous post: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/099123.htm...
The main purpose of this patchset is to fix an issue that a transaction can include MIDI bytes in the next message. The last patch solves this issue, and the first 4 patches are for better shape of code including a small fixes.
Changes:
- add buffer alias
- refine local variables
Applies all patches now. Thanks.
Thanks.
In this developing period, I'll post some patches for refactoring of fireworks and bebob modules till this weekend, then finish my work for 4.4.
Regards
Takashi Sakamoto
Takashi
Takashi Sakamoto (5): ALSA: firewire-tascam: remove buffer initialization in driver side ALSA: firewire-tascam: change type of valiables according to function prototype ALSA: firewire-tascam: use better name for local variables to describe their intension ALSA: firewire-tascam: fix loop condition with some readable variables ALSA: firewire-tascam: clear extra MIDI byte in an asynchronous transaction
sound/firewire/tascam/tascam-transaction.c | 73 +++++++++++++++++------------- 1 file changed, 41 insertions(+), 32 deletions(-)
-- 2.1.4
participants (2)
-
Takashi Iwai -
Takashi Sakamoto