Re: [mISDN PATCH v2 09/19] Fix TEI and SAPI handling

[Sam Ravnborg <sam@ravnborg.org>]

Hi Karsten.

Flash from the past looking at some ISDN layer2.
Some comments below.

	Sam

>  struct layer2 *
> -create_l2(struct mISDNchannel *ch, u_int protocol, u_long options, u_long arg)
> +create_l2(struct mISDNchannel *ch, u_int protocol, u_long options, u_int tei,
> +		u_int sapi)

Unrealted to this specific patch...
Are there any specific reason why mISDN prefer bsd style (u_int)
rather then linux style (u32)?

>  
>  static struct layer2 *
> -create_new_tei(struct manager *mgr, int tei)
> +create_new_tei(struct manager *mgr, int tei, int sapi)
>  {

Here tei and sapi are passed as signed.
But valid sapi range is [0..63] and tei [0..127].
And create_l2 above uses unsigned.

This looks confusing.

>  	u_long		opt = 0;
>  	u_long		flags;
> @@ -791,7 +791,7 @@ create_new_tei(struct manager *mgr, int tei)
>  	if (mgr->ch.st->dev->Dprotocols
>  	  & ((1 << ISDN_P_TE_E1) | (1 << ISDN_P_NT_E1)))
>  		test_and_set_bit(OPTION_L2_PMX, &opt);
> -	l2 = create_l2(mgr->up, ISDN_P_LAPD_NT, (u_int)opt, (u_long)tei);
> +	l2 = create_l2(mgr->up, ISDN_P_LAPD_NT, (u_int)opt, tei, sapi);
>  	if (!l2) {
>  		printk(KERN_WARNING "%s:no memory for layer2\n", __func__);
>  		return NULL;
> @@ -839,12 +839,15 @@ new_tei_req(struct manager *mgr, u_char *dp)
>  	ri += dp[1];
>  	if (!mgr->up)
>  		goto denied;
> -	tei = get_free_tei(mgr);
> +	if (dp[3] != 0xff)
> +		tei = dp[3] >> 1; /* 3GPP TS 08.56 6.1.11.2 */
> +	else
> +		tei = get_free_tei(mgr);

You should check EA0 here before assuming that any values except
EA=1, tei=127 equals ptmp.


>  	if (tei < 0) {
>  		printk(KERN_WARNING "%s:No free tei\n", __func__);
>  		goto denied;
>  	}

So get_free_tei() may return a negative value indicating no free tei.
So that make my comment above void - but is this really the best way
to return an error. Possibly it is..


> -	l2 = create_new_tei(mgr, tei);
> +	l2 = create_new_tei(mgr, tei, 0);

In general I would prefer to read: SAPI_CALLCONTROL rahter than a hardcoded 0.

>  	if (!l2)
>  		goto denied;
>  	else
> @@ -976,8 +979,6 @@ create_teimgr(struct manager *mgr, struct channel_req *crq)
>  			__func__, dev_name(&mgr->ch.st->dev->dev),
>  			crq->protocol, crq->adr.dev, crq->adr.channel,
>  			crq->adr.sapi, crq->adr.tei);
> -	if (crq->adr.sapi != 0) /* not supported yet */
> -		return -EINVAL;
>  	if (crq->adr.tei > GROUP_TEI)
>  		return -EINVAL;
>  	if (crq->adr.tei < 64)
> @@ -1025,7 +1026,7 @@ create_teimgr(struct manager *mgr, struct channel_req *crq)
>  		return 0;
>  	}
>  	l2 = create_l2(crq->ch, crq->protocol, (u_int)opt,
> -		(u_long)crq->adr.tei);
> +		crq->adr.tei, crq->adr.sapi);
>  	if (!l2)
>  		return -ENOMEM;
>  	l2->tm = kzalloc(sizeof(struct teimgr), GFP_KERNEL);
> @@ -1166,7 +1167,7 @@ static int
>  check_data(struct manager *mgr, struct sk_buff *skb)
>  {
>  	struct mISDNhead	*hh =  mISDN_HEAD_P(skb);
> -	int			ret, tei;
> +	int			ret, tei, sapi;
>  	struct layer2		*l2;
>  
>  	if (*debug & DEBUG_L2_CTRL)
> @@ -1178,18 +1179,16 @@ check_data(struct manager *mgr, struct sk_buff *skb)
>  		return -ENOTCONN;
>  	if (skb->len != 3)
>  		return -ENOTCONN;
> -	if (skb->data[0] != 0)
> -		/* only SAPI 0 command */
> -		return -ENOTCONN;
> +	sapi = skb->data[0] >> 2;

PReviously there was an implicit check of EA0.
This is missed not that you read sapi and discard the two lower bits.

I also wonder if you remeber to check the command/response bit.
That was implicitly tested to be 0 before and also ignored now.


>  	if (!(skb->data[1] & 1)) /* invalid EA1 */
>  		return -EINVAL;
> -	tei = skb->data[1] >> 0;
> +	tei = skb->data[1] >> 1;

This looks like a bug-fix...

>  	if (tei > 63) /* not a fixed tei */
>  		return -ENOTCONN;
>  	if ((skb->data[2] & ~0x10) != SABME)
>  		return -ENOTCONN;
>  	/* We got a SABME for a fixed TEI */
> -	l2 = create_new_tei(mgr, tei);
> +	l2 = create_new_tei(mgr, tei, sapi);
>  	if (!l2)
>  		return -ENOMEM;
>  	ret = l2->ch.send(&l2->ch, skb);