[PATCH 0/3] fix sending the same MIDI bytes in two transactions

[PATCH 0/3] fix sending the same MIDI bytes in two transactions

[Takashi Sakamoto]

Hi,

In former commits, MIDI functionality is added to TASCAM driver, while
it includes a bug to send the same MIDI bytes in two continuous
transactions.

This patchset is to fix this bug, including some code improvements.

Takashi Sakamoto (3):
  ALSA: tascam: remove buffer initialization in driver side
  ALSA: tascam: improve loop condition to filling MIDI bytes in
    transaction
  ALSA: tascam: clear extra MIDI byte of asynchronous transaction

 sound/firewire/tascam/tascam-transaction.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.1.4

[PATCH 1/3] ALSA: tascam: remove buffer initialization in driver side

[Takashi Sakamoto]

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 1c9a88b..7fcee81 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;
-- 
2.1.4

[PATCH 2/3] ALSA: tascam: improve loop condition to filling MIDI bytes in transaction

[Takashi Sakamoto]

It includes meaningless condition. This commit improve it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/tascam/tascam-transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
index 7fcee81..4ffaa8a 100644
--- a/sound/firewire/tascam/tascam-transaction.c
+++ b/sound/firewire/tascam/tascam-transaction.c
@@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
 	/* 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 < len + 1; ++i) {
 			if (buf[i] == 0xf7) {
 				tscm->on_sysex[port] = false;
 				break;
-- 
2.1.4

[PATCH 3/3] ALSA: tascam: clear extra MIDI byte of asynchronous transaction

[Takashi Sakamoto]

When MIDI buffer stores two or more MIDI messages, TASCAM driver
transfers asynchronous transactions including one MIDI message and
extra bytes in second MIDI message.

This commit fixes this bug by clear needless bytes in the buffer.

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

diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
index 4ffaa8a..4bedfe4 100644
--- a/sound/firewire/tascam/tascam-transaction.c
+++ b/sound/firewire/tascam/tascam-transaction.c
@@ -130,6 +130,9 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
 		buf[0] = (port << 4) | (buf[1] >> 4);
 	}
 
+	if (len < 3)
+		memset(buf + 1, 0, 3 - len);
+
 	return len;
 }
 
-- 
2.1.4

Re: [PATCH 2/3] ALSA: tascam: improve loop condition to filling MIDI bytes in transaction

[Takashi Iwai]

On Sun, 18 Oct 2015 12:02:17 +0200,
Takashi Sakamoto wrote:
> 
> It includes meaningless condition. This commit improve it.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/firewire/tascam/tascam-transaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
> index 7fcee81..4ffaa8a 100644
> --- a/sound/firewire/tascam/tascam-transaction.c
> +++ b/sound/firewire/tascam/tascam-transaction.c
> @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
>  	/* 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 < len + 1; ++i) {

This doesn't look like an improvement...
The fix should be rather like:
		for (i = 1; i < 4 && i < len; ++i) {


Takashi

Re: [PATCH 2/3] ALSA: tascam: improve loop condition to filling MIDI bytes in transaction

[Takashi Sakamoto]

On Oct 18 2015 19:10, Takashi Iwai wrote:
> On Sun, 18 Oct 2015 12:02:17 +0200,
> Takashi Sakamoto wrote:
>>
>> It includes meaningless condition. This commit improve it.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/firewire/tascam/tascam-transaction.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
>> index 7fcee81..4ffaa8a 100644
>> --- a/sound/firewire/tascam/tascam-transaction.c
>> +++ b/sound/firewire/tascam/tascam-transaction.c
>> @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
>>  	/* 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 < len + 1; ++i) {
> 
> This doesn't look like an improvement...
> The fix should be rather like:
> 		for (i = 1; i < 4 && i < len; ++i) {

The 'len' variable is assigned to the return value of
snd_rawmidi_transmit_peek(). The third argument to call this function is
3, thus the value of this variable equals to or is less than 3.

The aim of this loop is to find EOX (end of exclusive) byte in retrieved
bytes, then break the loop.The index goes from 1 to the end of retrieved
bytes. Thus, 'i < 4 || i < len' is not so proper, here.


Thanks

Takashi Sakamoto

Re: [PATCH 2/3] ALSA: tascam: improve loop condition to filling MIDI bytes in transaction

[Takashi Iwai]

On Sun, 18 Oct 2015 12:51:33 +0200,
Takashi Sakamoto wrote:
> 
> On Oct 18 2015 19:10, Takashi Iwai wrote:
> > On Sun, 18 Oct 2015 12:02:17 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> It includes meaningless condition. This commit improve it.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >> ---
> >>  sound/firewire/tascam/tascam-transaction.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
> >> index 7fcee81..4ffaa8a 100644
> >> --- a/sound/firewire/tascam/tascam-transaction.c
> >> +++ b/sound/firewire/tascam/tascam-transaction.c
> >> @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
> >>  	/* 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 < len + 1; ++i) {
> > 
> > This doesn't look like an improvement...
> > The fix should be rather like:
> > 		for (i = 1; i < 4 && i < len; ++i) {
> 
> The 'len' variable is assigned to the return value of
> snd_rawmidi_transmit_peek(). The third argument to call this function is
> 3, thus the value of this variable equals to or is less than 3.
> 
> The aim of this loop is to find EOX (end of exclusive) byte in retrieved
> bytes, then break the loop.The index goes from 1 to the end of retrieved
> bytes. Thus, 'i < 4 || i < len' is not so proper, here.

Then the patch description is incorrect.  You actually fix something
that was a buggy behavior.

Overall, the code is confusing because the real data buffer begins at
offset 1 and len doesn't mean the size of the values written to buf[].
Maybe it's better to be like:
	char *msg = buf + 1;

and use msg[x] instead of buf[1+x].  Then len really corresponds to
the size of msg[].


Takashi

Re: [PATCH 2/3] ALSA: tascam: improve loop condition to filling MIDI bytes in transaction

[Takashi Sakamoto]

On Oct 18 2015 21:00, Takashi Iwai wrote:
> On Sun, 18 Oct 2015 12:51:33 +0200,
> Takashi Sakamoto wrote:
>>
>> On Oct 18 2015 19:10, Takashi Iwai wrote:
>>> On Sun, 18 Oct 2015 12:02:17 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> It includes meaningless condition. This commit improve it.
>>>>
>>>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>>>> ---
>>>>  sound/firewire/tascam/tascam-transaction.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
>>>> index 7fcee81..4ffaa8a 100644
>>>> --- a/sound/firewire/tascam/tascam-transaction.c
>>>> +++ b/sound/firewire/tascam/tascam-transaction.c
>>>> @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
>>>>  	/* 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 < len + 1; ++i) {
>>>
>>> This doesn't look like an improvement...
>>> The fix should be rather like:
>>> 		for (i = 1; i < 4 && i < len; ++i) {
>>
>> The 'len' variable is assigned to the return value of
>> snd_rawmidi_transmit_peek(). The third argument to call this function is
>> 3, thus the value of this variable equals to or is less than 3.
>>
>> The aim of this loop is to find EOX (end of exclusive) byte in retrieved
>> bytes, then break the loop.The index goes from 1 to the end of retrieved
>> bytes. Thus, 'i < 4 || i < len' is not so proper, here.
> 
> Then the patch description is incorrect.  You actually fix something
> that was a buggy behavior.

It's not so buggy behaviour because the loop continue to seek the buffer
till its end (4th byte), always. But this is against my intention to
stop the seek in the end of retrieved byte.

Anyway, the code is not good.

> Overall, the code is confusing because the real data buffer begins at
> offset 1 and len doesn't mean the size of the values written to buf[].
> Maybe it's better to be like:
> 	char *msg = buf + 1;
> 
> and use msg[x] instead of buf[1+x].  Then len really corresponds to
> the size of msg[].

That's a good idea. OK, I'll post the revised version, later.


Thanks

Takashi Sakamoto