[PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Sakamoto]

In IEC 61883-6, 8 MIDI data streams are multiplexed into single
MIDI conformant data channel. The index of stream is calculated by
modulo 8 of the value of data block counter.

In fireworks, the value of data block counter in CIP header has a quirk
with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
IEC 61883-1/6 packet streaming engine to miss detection of MIDI
messages.

This commit fixes the miss detection to modify the value of data block
counter for the modulo calculation.

For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
fireworks/firewire-lib: add support for recent firmware quirk") in Linux
kernel v4.2. There're many changes since the commit.  This fix can be
backported to Linux kernel v4.4 or later. I tagged a base commit to the
backport for your convenience.

Besides, my work for Linux kernel v5.3 brings heavy code refactoring and
some structure members are renamed in 'sound/firewire/amdtp-stream.h'.
The content of this patch brings conflict when merging -rc tree with
this patch to the latest tree. I request maintainers to solve the
conflict by replacing 'tx_first_dbc' with 'ctx_data.tx.first_dbc'.

Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
Cc: <stable@vger.kernel.org> # v4.4+
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-am824.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/firewire/amdtp-am824.c b/sound/firewire/amdtp-am824.c
index 4210e5c6262e..4d677fcb4fc2 100644
--- a/sound/firewire/amdtp-am824.c
+++ b/sound/firewire/amdtp-am824.c
@@ -321,6 +321,7 @@ static void read_midi_messages(struct amdtp_stream *s,
 	u8 *b;
 
 	for (f = 0; f < frames; f++) {
+		port = (8 - s->tx_first_dbc + s->data_block_counter + f) % 8;
 		port = (s->data_block_counter + f) % 8;
 		b = (u8 *)&buffer[p->midi_position];
 
-- 
2.20.1

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Iwai]

On Mon, 01 Jul 2019 12:59:27 +0200,
Takashi Sakamoto wrote:
> 
> In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> MIDI conformant data channel. The index of stream is calculated by
> modulo 8 of the value of data block counter.
> 
> In fireworks, the value of data block counter in CIP header has a quirk
> with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> messages.
> 
> This commit fixes the miss detection to modify the value of data block
> counter for the modulo calculation.
> 
> For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> kernel v4.2. There're many changes since the commit.  This fix can be
> backported to Linux kernel v4.4 or later. I tagged a base commit to the
> backport for your convenience.
> 
> Besides, my work for Linux kernel v5.3 brings heavy code refactoring and
> some structure members are renamed in 'sound/firewire/amdtp-stream.h'.
> The content of this patch brings conflict when merging -rc tree with
> this patch to the latest tree. I request maintainers to solve the
> conflict by replacing 'tx_first_dbc' with 'ctx_data.tx.first_dbc'.
> 
> Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Thanks, applied.


Takashi

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Sakamoto]

On Mon, Jul 01, 2019 at 04:14:02PM +0200, Takashi Iwai wrote:
> On Mon, 01 Jul 2019 12:59:27 +0200,
> Takashi Sakamoto wrote:
> > 
> > In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> > MIDI conformant data channel. The index of stream is calculated by
> > modulo 8 of the value of data block counter.
> > 
> > In fireworks, the value of data block counter in CIP header has a quirk
> > with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> > IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> > messages.
> > 
> > This commit fixes the miss detection to modify the value of data block
> > counter for the modulo calculation.
> > 
> > For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> > fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> > kernel v4.2. There're many changes since the commit.  This fix can be
> > backported to Linux kernel v4.4 or later. I tagged a base commit to the
> > backport for your convenience.
> > 
> > Besides, my work for Linux kernel v5.3 brings heavy code refactoring and
> > some structure members are renamed in 'sound/firewire/amdtp-stream.h'.
> > The content of this patch brings conflict when merging -rc tree with
> > this patch to the latest tree. I request maintainers to solve the
> > conflict by replacing 'tx_first_dbc' with 'ctx_data.tx.first_dbc'.
> > 
> > Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> > Cc: <stable@vger.kernel.org> # v4.4+
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> Thanks, applied.

Thanks for your application, however I found my mistake in this patch.
Would you please reset your application if possible?

diff --git a/sound/firewire/amdtp-am824.c b/sound/firewire/amdtp-am824.c
index 4210e5c6262e..4d677fcb4fc2 100644
--- a/sound/firewire/amdtp-am824.c
+++ b/sound/firewire/amdtp-am824.c
@@ -321,6 +321,7 @@ static void read_midi_messages(struct amdtp_stream *s,
        u8 *b;
 
        for (f = 0; f < frames; f++) {
+               port = (8 - s->tx_first_dbc + s->data_block_counter + f) % 8;
                port = (s->data_block_counter + f) % 8;
                b = (u8 *)&buffer[p->midi_position];

Just inserting the above line has no meaning itself...


Thanks

Takashi Sakamoto

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Iwai]

On Mon, 01 Jul 2019 16:23:05 +0200,
Takashi Sakamoto wrote:
> 
> On Mon, Jul 01, 2019 at 04:14:02PM +0200, Takashi Iwai wrote:
> > On Mon, 01 Jul 2019 12:59:27 +0200,
> > Takashi Sakamoto wrote:
> > > 
> > > In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> > > MIDI conformant data channel. The index of stream is calculated by
> > > modulo 8 of the value of data block counter.
> > > 
> > > In fireworks, the value of data block counter in CIP header has a quirk
> > > with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> > > IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> > > messages.
> > > 
> > > This commit fixes the miss detection to modify the value of data block
> > > counter for the modulo calculation.
> > > 
> > > For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> > > fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> > > kernel v4.2. There're many changes since the commit.  This fix can be
> > > backported to Linux kernel v4.4 or later. I tagged a base commit to the
> > > backport for your convenience.
> > > 
> > > Besides, my work for Linux kernel v5.3 brings heavy code refactoring and
> > > some structure members are renamed in 'sound/firewire/amdtp-stream.h'.
> > > The content of this patch brings conflict when merging -rc tree with
> > > this patch to the latest tree. I request maintainers to solve the
> > > conflict by replacing 'tx_first_dbc' with 'ctx_data.tx.first_dbc'.
> > > 
> > > Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> > > Cc: <stable@vger.kernel.org> # v4.4+
> > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > 
> > Thanks, applied.
> 
> Thanks for your application, however I found my mistake in this patch.
> Would you please reset your application if possible?
> 
> diff --git a/sound/firewire/amdtp-am824.c b/sound/firewire/amdtp-am824.c
> index 4210e5c6262e..4d677fcb4fc2 100644
> --- a/sound/firewire/amdtp-am824.c
> +++ b/sound/firewire/amdtp-am824.c
> @@ -321,6 +321,7 @@ static void read_midi_messages(struct amdtp_stream *s,
>         u8 *b;
>  
>         for (f = 0; f < frames; f++) {
> +               port = (8 - s->tx_first_dbc + s->data_block_counter + f) % 8;
>                 port = (s->data_block_counter + f) % 8;
>                 b = (u8 *)&buffer[p->midi_position];
> 
> Just inserting the above line has no meaning itself...

Ah yes.  OK, will reset the repo.  Please resubmit the fix patch.


Takashi

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Sakamoto]

Hi,

On Mon, Jul 01, 2019 at 04:26:51PM +0200, Takashi Iwai wrote:
> On Mon, 01 Jul 2019 16:23:05 +0200,
> Takashi Sakamoto wrote:
> > 
> > On Mon, Jul 01, 2019 at 04:14:02PM +0200, Takashi Iwai wrote:
> > > On Mon, 01 Jul 2019 12:59:27 +0200,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> > > > MIDI conformant data channel. The index of stream is calculated by
> > > > modulo 8 of the value of data block counter.
> > > > 
> > > > In fireworks, the value of data block counter in CIP header has a quirk
> > > > with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> > > > IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> > > > messages.
> > > > 
> > > > This commit fixes the miss detection to modify the value of data block
> > > > counter for the modulo calculation.
> > > > 
> > > > For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> > > > fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> > > > kernel v4.2. There're many changes since the commit.  This fix can be
> > > > backported to Linux kernel v4.4 or later. I tagged a base commit to the
> > > > backport for your convenience.
> > > > 
> > > > Besides, my work for Linux kernel v5.3 brings heavy code refactoring and
> > > > some structure members are renamed in 'sound/firewire/amdtp-stream.h'.
> > > > The content of this patch brings conflict when merging -rc tree with
> > > > this patch to the latest tree. I request maintainers to solve the
> > > > conflict by replacing 'tx_first_dbc' with 'ctx_data.tx.first_dbc'.
> > > > 
> > > > Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> > > > Cc: <stable@vger.kernel.org> # v4.4+
> > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > 
> > > Thanks, applied.
> > 
> > Thanks for your application, however I found my mistake in this patch.
> > Would you please reset your application if possible?
> > 
> > diff --git a/sound/firewire/amdtp-am824.c b/sound/firewire/amdtp-am824.c
> > index 4210e5c6262e..4d677fcb4fc2 100644
> > --- a/sound/firewire/amdtp-am824.c
> > +++ b/sound/firewire/amdtp-am824.c
> > @@ -321,6 +321,7 @@ static void read_midi_messages(struct amdtp_stream *s,
> >         u8 *b;
> >  
> >         for (f = 0; f < frames; f++) {
> > +               port = (8 - s->tx_first_dbc + s->data_block_counter + f) % 8;
> >                 port = (s->data_block_counter + f) % 8;
> >                 b = (u8 *)&buffer[p->midi_position];
> > 
> > Just inserting the above line has no meaning itself...
> 
> Ah yes.  OK, will reset the repo.  Please resubmit the fix patch.

Thanks for your accepting the reset. I'm ease to hear it ;)
I'll post the revised patch later with enough pre-check.


Thanks

Takashi Sakamoto

[PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Sakamoto]

In IEC 61883-6, 8 MIDI data streams are multiplexed into single
MIDI conformant data channel. The index of stream is calculated by
modulo 8 of the value of data block counter.

In fireworks, the value of data block counter in CIP header has a quirk
with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
IEC 61883-1/6 packet streaming engine to miss detection of MIDI
messages.

This commit fixes the miss detection to modify the value of data block
counter for the modulo calculation.

For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
fireworks/firewire-lib: add support for recent firmware quirk") in Linux
kernel v4.2. There're many changes since the commit.  This fix can be
backported to Linux kernel v4.4 or later. I tagged a base commit to the
backport for your convenience.

Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
Cc: <stable@vger.kernel.org> # v4.4+
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-am824.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/amdtp-am824.c b/sound/firewire/amdtp-am824.c
index 7019a2143581..623d014c0e7e 100644
--- a/sound/firewire/amdtp-am824.c
+++ b/sound/firewire/amdtp-am824.c
@@ -321,7 +321,7 @@ static void read_midi_messages(struct amdtp_stream *s,
 	u8 *b;
 
 	for (f = 0; f < frames; f++) {
-		port = (s->data_block_counter + f) % 8;
+		port = (8 - s->ctx_data.tx.first_dbc + s->data_block_counter + f) % 8;
 		b = (u8 *)&buffer[p->midi_position];
 
 		len = b[0] - 0x80;
-- 
2.20.1

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Iwai]

On Fri, 28 Jun 2019 07:21:58 +0200,
Takashi Sakamoto wrote:
> 
> In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> MIDI conformant data channel. The index of stream is calculated by
> modulo 8 of the value of data block counter.
> 
> In fireworks, the value of data block counter in CIP header has a quirk
> with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> messages.
> 
> This commit fixes the miss detection to modify the value of data block
> counter for the modulo calculation.
> 
> For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> kernel v4.2. There're many changes since the commit.  This fix can be
> backported to Linux kernel v4.4 or later. I tagged a base commit to the
> backport for your convenience.
> 
> Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

This doesn't seem applicable to the latest 5.2-rc tree due to your
recent refactoring.  Could you resubmit the fix for 5.2?  I'll resolve
the merge conflict in my side.


thanks,

Takashi

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Sakamoto]

Hi,

On Fri, Jun 28, 2019, at 17:53, Takashi Iwai wrote:
> On Fri, 28 Jun 2019 07:21:58 +0200,
> Takashi Sakamoto wrote:
> > 
> > In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> > MIDI conformant data channel. The index of stream is calculated by
> > modulo 8 of the value of data block counter.
> > 
> > In fireworks, the value of data block counter in CIP header has a quirk
> > with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> > IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> > messages.
> > 
> > This commit fixes the miss detection to modify the value of data block
> > counter for the modulo calculation.
> > 
> > For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> > fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> > kernel v4.2. There're many changes since the commit.  This fix can be
> > backported to Linux kernel v4.4 or later. I tagged a base commit to the
> > backport for your convenience.
> > 
> > Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> > Cc: <stable@vger.kernel.org> # v4.4+
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> This doesn't seem applicable to the latest 5.2-rc tree due to your
> recent refactoring.  Could you resubmit the fix for 5.2?  I'll resolve
> the merge conflict in my side.

Mmm. Do you actually encounter any conflict when applying this patch to
your v5.2 tree?

This patch includes changes for `sound/firewire/amdtp-am824.c`. On the other
hand, my recent work is mainly for `sound/firewire/amdtp-stream.c`. Actually,
the last change for `amdtp-am824.c` was done 2017-10-25.
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/sound/firewire/amdtp-am824.c?h=for-linus


Thanks


Takashi Sakamoto

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Iwai]

On Fri, 28 Jun 2019 09:34:00 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Fri, Jun 28, 2019, at 17:53, Takashi Iwai wrote:
> > On Fri, 28 Jun 2019 07:21:58 +0200,
> > Takashi Sakamoto wrote:
> > > 
> > > In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> > > MIDI conformant data channel. The index of stream is calculated by
> > > modulo 8 of the value of data block counter.
> > > 
> > > In fireworks, the value of data block counter in CIP header has a quirk
> > > with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> > > IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> > > messages.
> > > 
> > > This commit fixes the miss detection to modify the value of data block
> > > counter for the modulo calculation.
> > > 
> > > For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> > > fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> > > kernel v4.2. There're many changes since the commit.  This fix can be
> > > backported to Linux kernel v4.4 or later. I tagged a base commit to the
> > > backport for your convenience.
> > > 
> > > Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> > > Cc: <stable@vger.kernel.org> # v4.4+
> > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > 
> > This doesn't seem applicable to the latest 5.2-rc tree due to your
> > recent refactoring.  Could you resubmit the fix for 5.2?  I'll resolve
> > the merge conflict in my side.
> 
> Mmm. Do you actually encounter any conflict when applying this patch to
> your v5.2 tree?
> 
> This patch includes changes for `sound/firewire/amdtp-am824.c`. On the other
> hand, my recent work is mainly for `sound/firewire/amdtp-stream.c`. Actually,
> the last change for `amdtp-am824.c` was done 2017-10-25.
> https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/sound/firewire/amdtp-am824.c?h=for-linus

It's not about file conflicts but the compilation fails after the
patch.
sound/firewire/amdtp-am824.c: In function ‘read_midi_messages’:
sound/firewire/amdtp-am824.c:324:16: error: ‘struct amdtp_stream’ has no member named ‘ctx_data’
   port = (8 - s->ctx_data.tx.first_dbc + s->data_block_counter + f) % 8;
                ^~


thanks,

Takashi

Re: [PATCH] ALSA: firewire-lib/fireworks: fix miss detection of received MIDI messages

[Takashi Sakamoto]

Hi,

On Sat, Jun 29, 2019, at 00:44, Takashi Iwai wrote:
> On Fri, 28 Jun 2019 09:34:00 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > On Fri, Jun 28, 2019, at 17:53, Takashi Iwai wrote:
> > > On Fri, 28 Jun 2019 07:21:58 +0200,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > In IEC 61883-6, 8 MIDI data streams are multiplexed into single
> > > > MIDI conformant data channel. The index of stream is calculated by
> > > > modulo 8 of the value of data block counter.
> > > > 
> > > > In fireworks, the value of data block counter in CIP header has a quirk
> > > > with firmware version v5.0.0, v5.7.3 and v5.8.0. This brings ALSA
> > > > IEC 61883-1/6 packet streaming engine to miss detection of MIDI
> > > > messages.
> > > > 
> > > > This commit fixes the miss detection to modify the value of data block
> > > > counter for the modulo calculation.
> > > > 
> > > > For maintainers, this bug exists since a commit 18f5ed365d3f ("ALSA:
> > > > fireworks/firewire-lib: add support for recent firmware quirk") in Linux
> > > > kernel v4.2. There're many changes since the commit.  This fix can be
> > > > backported to Linux kernel v4.4 or later. I tagged a base commit to the
> > > > backport for your convenience.
> > > > 
> > > > Fixes: df075feefbd3 ("ALSA: firewire-lib: complete AM824 data block processing layer")
> > > > Cc: <stable@vger.kernel.org> # v4.4+
> > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > 
> > > This doesn't seem applicable to the latest 5.2-rc tree due to your
> > > recent refactoring.  Could you resubmit the fix for 5.2?  I'll resolve
> > > the merge conflict in my side.
> > 
> > Mmm. Do you actually encounter any conflict when applying this patch to
> > your v5.2 tree?
> > 
> > This patch includes changes for `sound/firewire/amdtp-am824.c`. On the other
> > hand, my recent work is mainly for `sound/firewire/amdtp-stream.c`. Actually,
> > the last change for `amdtp-am824.c` was done 2017-10-25.
> > https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/sound/firewire/amdtp-am824.c?h=for-linus
> 
> It's not about file conflicts but the compilation fails after the
> patch.
> sound/firewire/amdtp-am824.c: In function ‘read_midi_messages’:
> sound/firewire/amdtp-am824.c:324:16: error: ‘struct amdtp_stream’ has 
> no member named ‘ctx_data’
>    port = (8 - s->ctx_data.tx.first_dbc + s->data_block_counter + f) % 
> 8;
>                 ^~

Oops, now I got it... I just checked its application but should have had
compile test with old trees.

But I'm in short vacation. The revised patch will be posted next Monday,
sorry.


Thanks

Takashi Sakamoto