[PATCH] isdn/i4l: Fix DLE handling for i4l-audio

[PATCH] isdn/i4l: Fix DLE handling for i4l-audio

[Matthias Goebl]

The DLE handling in i4l-audio seems to be broken.

It produces spurious DLEs so asterisk 1.2.24 with chan_modem_i4l
gets irritated, the error message is:
"chan_modem_i4l.c:450 i4l_read: Value of escape is ^ (17)".
-> There shouldn't be a DLE-^.
If a spurious DLE-ETX occurs, the audio connection even dies.
I use a "AVM Fritz!PCI" isdn card.

I found two issues that only appear if ISDN_AUDIO_SKB_DLECOUNT(skb) > 0:
- The loop in isdn_tty.c:isdn_tty_try_read() doesn't escape a DLE if it's
  the last character.

- The loop in isdn_common.c:isdn_readbchan_tty() doesn't copy its characters,
  it only remembers the last one ("last = *p;").

  Compare it with the loop in isdn_common.c:isdn_readbchan(), that *does*
  copy them ("*cp++ = *p;") correctly.
  The special handling of the "last" character made it more difficult.
  I compared it to linux-2.4.19: There was no "last"-handling and both loops
  did escape and copy all characters correctly.


Signed-off-by: Matthias Goebl <matthias.goebl@goebl.net>

--- linux-2.6.23.12.orig/drivers/isdn/i4l/isdn_common.c	2007-12-22 21:13:49.000000000 +0100
+++ linux-2.6.23.12/drivers/isdn/i4l/isdn_common.c	2007-12-26 11:36:52.000000000 +0100
@@ -914,6 +914,9 @@
 			dflag = 0;
 			count_pull = count_put = 0;
 			while ((count_pull < skb->len) && (len > 0)) {
+				/* push every character but the last to the tty buffer directly */
+				if ( count_put )
+					tty_insert_flip_char(tty, last, TTY_NORMAL);
 				len--;
 				if (dev->drv[di]->DLEflag & DLEmask) {
 					last = DLE;
--- linux-2.6.23.12.orig/drivers/isdn/i4l/isdn_tty.c	2007-12-22 21:13:49.000000000 +0100
+++ linux-2.6.23.12/drivers/isdn/i4l/isdn_tty.c	2007-12-26 11:37:18.000000000 +0100
@@ -85,6 +85,8 @@
 								tty_insert_flip_char(tty, DLE, 0);
 							tty_insert_flip_char(tty, *dp++, 0);
 						}
+						if (*dp == DLE)
+							tty_insert_flip_char(tty, DLE, 0);
 						last = *dp;
 					} else {
 #endif

Re: [PATCH] isdn/i4l: Fix DLE handling for i4l-audio

[David Miller]

From: Matthias Goebl <matthias@goebl.net>
Date: Thu, 3 Jan 2008 23:00:03 +0100

> --- linux-2.6.23.12.orig/drivers/isdn/i4l/isdn_tty.c	2007-12-22 21:13:49.000000000 +0100
> +++ linux-2.6.23.12/drivers/isdn/i4l/isdn_tty.c	2007-12-26 11:37:18.000000000 +0100
> @@ -85,6 +85,8 @@
>  								tty_insert_flip_char(tty, DLE, 0);
>  							tty_insert_flip_char(tty, *dp++, 0);
>  						}
> +						if (*dp == DLE)
> +							tty_insert_flip_char(tty, DLE, 0);
>  						last = *dp;
>  					} else {
>  #endif

I'm not sure this part is correct.

Here, *dp will be assigned to 'last'.

The rest of the code (after the else block) then reads:

					if (info->emu.mdmreg[REG_CPPP] & BIT_CPPP)
						tty_insert_flip_char(tty, last, 0xFF);
					else
						tty_insert_flip_char(tty, last, TTY_NORMAL);

which should push that character out to the TTY.

With your change we will push a DLE out twice in such a case,
and that doesn't seem right.

Re: [PATCH] isdn/i4l: Fix DLE handling for i4l-audio

[Matthias Goebl]

On Fri, Jan 04, 2008 at 02:14:37AM -0800, David Miller wrote:
> From: Matthias Goebl <matthias@goebl.net>
> Date: Thu, 3 Jan 2008 23:00:03 +0100
> 
> > --- linux-2.6.23.12.orig/drivers/isdn/i4l/isdn_tty.c	2007-12-22 21:13:49.000000000 +0100
> > +++ linux-2.6.23.12/drivers/isdn/i4l/isdn_tty.c	2007-12-26 11:37:18.000000000 +0100
> > @@ -85,6 +85,8 @@
> >  								tty_insert_flip_char(tty, DLE, 0);
> >  							tty_insert_flip_char(tty, *dp++, 0);
> >  						}
> > +						if (*dp == DLE)
> > +							tty_insert_flip_char(tty, DLE, 0);
> >  						last = *dp;
> >  					} else {
> >  #endif
> 
> I'm not sure this part is correct.
> 
> Here, *dp will be assigned to 'last'.

The while (--l) {...} pushes+adds DLE for len-1 characters.
( while (l--) would process all len characters)
The last charater is assigned to "last" and pushed after the else block:

> The rest of the code (after the else block) then reads:
> 
> 					if (info->emu.mdmreg[REG_CPPP] & BIT_CPPP)
> 						tty_insert_flip_char(tty, last, 0xFF);
> 					else
> 						tty_insert_flip_char(tty, last, TTY_NORMAL);
> 
> which should push that character out to the TTY.

But this does not care about a DLE and doesn't escape that with another DLE.

> With your change we will push a DLE out twice in such a case,
> and that doesn't seem right.

For an audio data stream we have to escape DLE (ascii 16) by another DLE,
otherwise it is interpreted as command, as in Documentation/isdn/README.audio:
...  escape sequences defined, all using DLE (0x10) as Escape char:...
    <DLE><DLE>              Escape sequence for DLE in data stream.
    <DLE>0                  Touchtone "0" received.
...

Re: [PATCH] isdn/i4l: Fix DLE handling for i4l-audio

[David Miller]

From: Matthias Goebl <matthias@goebl.net>
Date: Fri, 4 Jan 2008 11:42:28 +0100

> But this does not care about a DLE and doesn't escape that with another DLE.

Now I understand, thank you.

I'll apply your patch.