lkml.org 
[lkml]   [2009]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [mISDN PATCH v2 09/19] Fix TEI and SAPI handling
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);



\
 
 \ /
  Last update: 2009-05-22 23:55    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site