[mISDN,v2,09/19] Fix TEI and SAPI handling

From: Andreas Eversberg <andreas@eversberg.eu>

Added SAPI value to use SAPIs different than 0.

Now fixed TEIs work in NT mode. This allows PTP endpoint to be connected
to PTMP ports together with other PTMP endpoints.

Signed-off-by: Andreas Eversberg <andreas@eversberg.eu>
Signed-off-by: Karsten Keil <keil@b1-systems.de>
---
 drivers/isdn/mISDN/layer2.c |   13 +++++++------
 drivers/isdn/mISDN/layer2.h |    2 +-
 drivers/isdn/mISDN/tei.c    |   25 ++++++++++++-------------
 3 files changed, 20 insertions(+), 20 deletions(-)

Hi Sam,

thanks for the review.

On Freitag, 22. Mai 2009 23:53:14 Sam Ravnborg wrote:
> 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)?

No, this was some historic process with the ISDN code.
I think, if we want change this we should make a separate  patch to change 
this in all mISDN code later, I usually prefer shorter type names as well :-).
So as rule of dumb:
- Use   uN/sN for all values with constant width N ?

What should be used on functions which are usually defined as int and
only return 0 or some negative ERROR code, I think int should be OK here ?

>
> >  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.

I think the signed was selected for error handling, but you are correct then 
it should be the same in all functions.

>
> >  	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.
>

make sense.

> >  	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..
>
We could define 255 as the error value, I will think about this, but in 
general I prefer negative values for error cases.

> > -	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.
>

Yes.

> >  	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.

Yes,  you are correct.

>
> >  	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...
>

Yes,  I think fixed TEI was never tested with P2MP,  I know
no device which really use it with SAPI 0, only in P2P we use a
fixed TEI  of 0.  Also the test suite used by certifications did
not check for correct fixed TEI handling, it only checks that a
fixed TEI is rejected in a dynamic TEI assignment.
With SAPI != 0 fixed TEIs are more common, so the bug was
detected now.

> >  	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);


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> >
> > Unrealted to this specific patch...
> > Are there any specific reason why mISDN prefer bsd style (u_int)
> > rather then linux style (u32)?
> 
> No, this was some historic process with the ISDN code.
> I think, if we want change this we should make a separate  patch to change 
> this in all mISDN code later, I usually prefer shorter type names as well :-).
> So as rule of dumb:
> - Use   uN/sN for all values with constant width N ?
yes

> What should be used on functions which are usually defined as int and
> only return 0 or some negative ERROR code, I think int should be OK here ?
yes - use int here.

> > >  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.
> 
> I think the signed was selected for error handling, but you are correct then 
> it should be the same in all functions.
The correct values are the range [0..127] so as you also imply
reserving all negative values for error codes seems like the best choice.

So as a matter of consistency you should use int for tei/sapi all over.
> >
> > 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..
> >
> We could define 255 as the error value, I will think about this, but in 
> general I prefer negative values for error cases.

Addressed above - agree.

> > >  	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...
> >
> 
> Yes,  I think fixed TEI was never tested with P2MP,  I know
> no device which really use it with SAPI 0, only in P2P we use a
> fixed TEI  of 0.  Also the test suite used by certifications did
> not check for correct fixed TEI handling, it only checks that a
> fixed TEI is rejected in a dynamic TEI assignment.
> With SAPI != 0 fixed TEIs are more common, so the bug was
> detected now.
Yep - I know we used fixed tei in ptmp configuration
for sapi=16 (packet handling aka X.25 over ISDN).

	Sam
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html