[PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors.

[PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors.

[Bas Peters]

This patchset tackles a variety of checkpatch errors. Some apparent errors were ommitted because fixing them would in no way contribute to readability.

Signed-off-by: Bas Peters <baspeters93@gmail.com>

Bas Peters (6):
  drivers: isdn: act2000: act2000_isa.c: Fix checkpatch errors
  drivers: isdn: act2000: capi.c: fix checkpatch errors
  drivers: isdn: act2000: remove assignments of variables in if
    conditions
  drivers: isdn: act2000: module.c: remove NULL-initialization of static
        variable.
  drivers: isdn: act2000: module.c: remove parenthesres around return   
     values.
  drivers: isdn: act2000: fix wrongly positioned brace.

 drivers/isdn/act2000/act2000_isa.c | 11 +++++----
 drivers/isdn/act2000/capi.c        |  9 +++++---
 drivers/isdn/act2000/module.c      | 47 +++++++++++++++++++++++---------------
 3 files changed, 41 insertions(+), 26 deletions(-)

-- 
2.1.0

[PATCH 1/6] drivers: isdn: act2000: act2000_isa.c: Fix checkpatch errors

[Bas Peters]

This patch adresses various checkpatch errors:
	3 assignments in if conditions
	1 return value enclosed in parenthesis

Signed-off-by: Bas Peters <baspeters93@gmail.com>
---
 drivers/isdn/act2000/act2000_isa.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/isdn/act2000/act2000_isa.c b/drivers/isdn/act2000/act2000_isa.c
index b5fad29..048507e 100644
--- a/drivers/isdn/act2000/act2000_isa.c
+++ b/drivers/isdn/act2000/act2000_isa.c
@@ -31,7 +31,8 @@ act2000_isa_reset(unsigned short portbase)
 	int serial = 0;
 
 	found = 0;
-	if ((reg = inb(portbase + ISA_COR)) != 0xff) {
+	reg = inb(portbase + ISA_COR);
+	if (reg != 0xff) {
 		outb(reg | ISA_COR_RESET, portbase + ISA_COR);
 		mdelay(10);
 		outb(reg, portbase + ISA_COR);
@@ -303,7 +304,8 @@ act2000_isa_send(act2000_card *card)
 	while (1) {
 		spin_lock_irqsave(&card->lock, flags);
 		if (!(card->sbuf)) {
-			if ((card->sbuf = skb_dequeue(&card->sndq))) {
+			card->sbuf = skb_dequeue(&card->sndq);
+			if (card->sbuf) {
 				card->ack_msg = card->sbuf->data;
 				msg = (actcapi_msg *)card->sbuf->data;
 				if ((msg->hdr.cmd.cmd == 0x86) &&
@@ -378,7 +380,8 @@ act2000_isa_getid(act2000_card *card)
 		printk(KERN_WARNING "act2000: Wrong Firmware-ID!\n");
 		return -EPROTO;
 	}
-	if ((p = strchr(fid.revision, '\n')))
+	p = strchr(fid.revision, '\n');
+	if (p)
 		*p = '\0';
 	printk(KERN_INFO "act2000: Firmware-ID: %s\n", fid.revision);
 	if (card->flags & ACT2000_FLAGS_IVALID) {
@@ -439,5 +442,5 @@ act2000_isa_download(act2000_card *card, act2000_ddef __user *cb)
 	}
 	kfree(buf);
 	msleep_interruptible(500);
-	return (act2000_isa_getid(card));
+	return act2000_isa_getid(card);
 }
-- 
2.1.0

[PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Bas Peters]

This patch fixes the following checkpatch errors:
	1. trailing statement
	1. assignment of variable in if condition
	1. incorrectly placed brace after function definition

Signed-off-by: Bas Peters <baspeters93@gmail.com>
---
 drivers/isdn/act2000/capi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/act2000/capi.c b/drivers/isdn/act2000/capi.c
index 3f66ca2..5d677e6 100644
--- a/drivers/isdn/act2000/capi.c
+++ b/drivers/isdn/act2000/capi.c
@@ -113,7 +113,8 @@ actcapi_chkhdr(act2000_card *card, actcapi_msghdr *hdr)
 			m->hdr.cmd.cmd = c;			\
 			m->hdr.cmd.subcmd = s;			\
 			m->hdr.msgnum = actcapi_nextsmsg(card); \
-		} else m = NULL;				\
+		} else
+			m = NULL;				\
 	}
 
 #define ACTCAPI_CHKSKB if (!skb) {					\
@@ -563,7 +564,8 @@ actcapi_data_b3_ind(act2000_card *card, struct sk_buff *skb) {
 	blocknr = msg->msg.data_b3_ind.blocknr;
 	skb_pull(skb, 19);
 	card->interface.rcvcallb_skb(card->myid, chan, skb);
-	if (!(skb = alloc_skb(11, GFP_ATOMIC))) {
+	skb = alloc_skb(11, GFP_ATOMIC);
+	if (!skb) {
 		printk(KERN_WARNING "actcapi: alloc_skb failed\n");
 		return 1;
 	}
@@ -990,7 +992,8 @@ actcapi_debug_dlpd(actcapi_dlpd *dlpd)
 }
 
 #ifdef DEBUG_DUMP_SKB
-static void dump_skb(struct sk_buff *skb) {
+static void dump_skb(struct sk_buff *skb)
+{
 	char tmp[80];
 	char *p = skb->data;
 	char *t = tmp;
-- 
2.1.0

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Sergei Shtylyov]

Hello.

On 02/07/2015 08:06 PM, Bas Peters wrote:

> This patch fixes the following checkpatch errors:
> 	1. trailing statement
> 	1. assignment of variable in if condition
> 	1. incorrectly placed brace after function definition

> Signed-off-by: Bas Peters <baspeters93@gmail.com>
> ---
>   drivers/isdn/act2000/capi.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)

> diff --git a/drivers/isdn/act2000/capi.c b/drivers/isdn/act2000/capi.c
> index 3f66ca2..5d677e6 100644
> --- a/drivers/isdn/act2000/capi.c
> +++ b/drivers/isdn/act2000/capi.c
> @@ -113,7 +113,8 @@ actcapi_chkhdr(act2000_card *card, actcapi_msghdr *hdr)
>   			m->hdr.cmd.cmd = c;			\
>   			m->hdr.cmd.subcmd = s;			\
>   			m->hdr.msgnum = actcapi_nextsmsg(card); \
> -		} else m = NULL;				\
> +		} else
> +			m = NULL;				\

    Documentation/CodingStyle has an extra rule for such case: *else* branch 
should also have {} since *if* branch has {}.

[...]

WNR, Sergei

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Bas Peters]

Thanks, will take that into account in future patches.

With kind regards,

Bas

2015-02-07 18:51 GMT+01:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
> On 02/07/2015 08:06 PM, Bas Peters wrote:
>
>> This patch fixes the following checkpatch errors:
>>         1. trailing statement
>>         1. assignment of variable in if condition
>>         1. incorrectly placed brace after function definition
>
>
>> Signed-off-by: Bas Peters <baspeters93@gmail.com>
>> ---
>>   drivers/isdn/act2000/capi.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>
>
>> diff --git a/drivers/isdn/act2000/capi.c b/drivers/isdn/act2000/capi.c
>> index 3f66ca2..5d677e6 100644
>> --- a/drivers/isdn/act2000/capi.c
>> +++ b/drivers/isdn/act2000/capi.c
>> @@ -113,7 +113,8 @@ actcapi_chkhdr(act2000_card *card, actcapi_msghdr
>> *hdr)
>>                         m->hdr.cmd.cmd = c;                     \
>>                         m->hdr.cmd.subcmd = s;                  \
>>                         m->hdr.msgnum = actcapi_nextsmsg(card); \
>> -               } else m = NULL;                                \
>> +               } else
>> +                       m = NULL;                               \
>
>
>    Documentation/CodingStyle has an extra rule for such case: *else* branch
> should also have {} since *if* branch has {}.
>
> [...]
>
> WNR, Sergei
>

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Julia Lawall]

On Sat, 7 Feb 2015, Sergei Shtylyov wrote:

> Hello.
> 
> On 02/07/2015 08:06 PM, Bas Peters wrote:
> 
> > This patch fixes the following checkpatch errors:
> > 	1. trailing statement
> > 	1. assignment of variable in if condition
> > 	1. incorrectly placed brace after function definition
> 
> > Signed-off-by: Bas Peters <baspeters93@gmail.com>
> > ---
> >   drivers/isdn/act2000/capi.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> > diff --git a/drivers/isdn/act2000/capi.c b/drivers/isdn/act2000/capi.c
> > index 3f66ca2..5d677e6 100644
> > --- a/drivers/isdn/act2000/capi.c
> > +++ b/drivers/isdn/act2000/capi.c
> > @@ -113,7 +113,8 @@ actcapi_chkhdr(act2000_card *card, actcapi_msghdr *hdr)
> >   			m->hdr.cmd.cmd = c;			\
> >   			m->hdr.cmd.subcmd = s;			\
> >   			m->hdr.msgnum = actcapi_nextsmsg(card); \
> > -		} else m = NULL;				\
> > +		} else

It looks like a \ was lost on this line.  It should have caused a compiler 
error.

julia

> > +			m = NULL;				\
> 
>    Documentation/CodingStyle has an extra rule for such case: *else* branch
> should also have {} since *if* branch has {}.
> 
> [...]
> 
> WNR, Sergei
> 
> 

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Bas Peters]

I must have missed something. I fixed it on my end. Do I now resend
the entire patchset or just this particular patch?

regards,

Bas

2015-02-07 19:02 GMT+01:00 Julia Lawall <julia.lawall@lip6.fr>:
> On Sat, 7 Feb 2015, Sergei Shtylyov wrote:
>
>> Hello.
>>
>> On 02/07/2015 08:06 PM, Bas Peters wrote:
>>
>> > This patch fixes the following checkpatch errors:
>> >     1. trailing statement
>> >     1. assignment of variable in if condition
>> >     1. incorrectly placed brace after function definition
>>
>> > Signed-off-by: Bas Peters <baspeters93@gmail.com>
>> > ---
>> >   drivers/isdn/act2000/capi.c | 9 ++++++---
>> >   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> > diff --git a/drivers/isdn/act2000/capi.c b/drivers/isdn/act2000/capi.c
>> > index 3f66ca2..5d677e6 100644
>> > --- a/drivers/isdn/act2000/capi.c
>> > +++ b/drivers/isdn/act2000/capi.c
>> > @@ -113,7 +113,8 @@ actcapi_chkhdr(act2000_card *card, actcapi_msghdr *hdr)
>> >                     m->hdr.cmd.cmd = c;                     \
>> >                     m->hdr.cmd.subcmd = s;                  \
>> >                     m->hdr.msgnum = actcapi_nextsmsg(card); \
>> > -           } else m = NULL;                                \
>> > +           } else
>
> It looks like a \ was lost on this line.  It should have caused a compiler
> error.
>
> julia
>
>> > +                   m = NULL;                               \
>>
>>    Documentation/CodingStyle has an extra rule for such case: *else* branch
>> should also have {} since *if* branch has {}.
>>
>> [...]
>>
>> WNR, Sergei
>>
>>

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Sergei Shtylyov]

On 02/07/2015 09:08 PM, Bas Peters wrote:

    Please don't top-post.

> I must have missed something. I fixed it on my end. Do I now resend
> the entire patchset or just this particular patch?

    The entire patch set, at least DaveM wants it this way.

> regards,

> Bas

WBR, Sergei

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Joe Perches]

On Sat, 2015-02-07 at 20:51 +0300, Sergei Shtylyov wrote:
> On 02/07/2015 08:06 PM, Bas Peters wrote:
> 
> > This patch fixes the following checkpatch errors:
> > 	1. trailing statement
> > 	1. assignment of variable in if condition
> > 	1. incorrectly placed brace after function definition

I wonder about the usefulness of these changes.

Does anyone still use these cards?

> > diff --git a/drivers/isdn/act2000/capi.c b/drivers/isdn/act2000/capi.c
[]
> > @@ -113,7 +113,8 @@ actcapi_chkhdr(act2000_card *card, actcapi_msghdr *hdr)
> >   			m->hdr.cmd.cmd = c;			\
> >   			m->hdr.cmd.subcmd = s;			\
> >   			m->hdr.msgnum = actcapi_nextsmsg(card); \
> > -		} else m = NULL;				\
> > +		} else
> > +			m = NULL;				\
> 
>     Documentation/CodingStyle has an extra rule for such case: *else* branch 
> should also have {} since *if* branch has {}.

The macro itself is already poor form.
-----
#define ACTCAPI_MKHDR(l, c, s) {				\
		skb = alloc_skb(l + 8, GFP_ATOMIC);		\
		if (skb) {					\
			m = (actcapi_msg *)skb_put(skb, l + 8); \
			m->hdr.len = l + 8;			\
			m->hdr.applicationID = 1;		\
			m->hdr.cmd.cmd = c;			\
			m->hdr.cmd.subcmd = s;			\
			m->hdr.msgnum = actcapi_nextsmsg(card); \
		} else m = NULL;				\
	}
-----

o hidden arguments skb, m and card
o missing do {} while around multi-statement macro

It'd probably be nicer to change the macro to a function
and pass m and card.

This would reduce the object size too.

But maybe any change here is not necessary.

If anything is done, I suggest something like:
---
 drivers/isdn/act2000/capi.c | 204 +++++++++++++++++++++++++-------------------
 1 file changed, 115 insertions(+), 89 deletions(-)

diff --git a/drivers/isdn/act2000/capi.c b/drivers/isdn/act2000/capi.c
index 3f66ca2..a7ecf51 100644
--- a/drivers/isdn/act2000/capi.c
+++ b/drivers/isdn/act2000/capi.c
@@ -104,29 +104,36 @@ actcapi_chkhdr(act2000_card *card, actcapi_msghdr *hdr)
 	return 0;
 }
 
-#define ACTCAPI_MKHDR(l, c, s) {				\
-		skb = alloc_skb(l + 8, GFP_ATOMIC);		\
-		if (skb) {					\
-			m = (actcapi_msg *)skb_put(skb, l + 8); \
-			m->hdr.len = l + 8;			\
-			m->hdr.applicationID = 1;		\
-			m->hdr.cmd.cmd = c;			\
-			m->hdr.cmd.subcmd = s;			\
-			m->hdr.msgnum = actcapi_nextsmsg(card); \
-		} else m = NULL;				\
-	}
+static struct sk_buff *actcapi_mkhdr(act2000_card *card, actcapi_msg **m,
+				     int len, int cmd, int subcmd)
+{
+	struct sk_buff *skb;
 
-#define ACTCAPI_CHKSKB if (!skb) {					\
-		printk(KERN_WARNING "actcapi: alloc_skb failed\n");	\
-		return;							\
-	}
+	len += 8;
 
-#define ACTCAPI_QUEUE_TX {				\
-		actcapi_debug_msg(skb, 1);		\
-		skb_queue_tail(&card->sndq, skb);	\
-		act2000_schedule_tx(card);		\
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (!skb) {
+		*m = NULL;
+		return NULL;
 	}
 
+	*m = (actcapi_msg *)skb_put(skb, len);
+	(*m)->hdr.len = len;
+	(*m)->hdr.applicationID = 1;
+	(*m)->hdr.cmd.cmd = cmd;
+	(*m)->hdr.cmd.subcmd = subcmd;
+	(*m)->hdr.msgnum = actcapi_nextsmsg(card);
+
+	return skb;
+}
+
+static void actcapi_queue_tx(act2000_card *card, struct sk_buff *skb)
+{
+	actcapi_debug_msg(skb, 1);
+	skb_queue_tail(&card->sndq, skb);
+	act2000_schedule_tx(card);
+}
+
 int
 actcapi_listen_req(act2000_card *card)
 {
@@ -137,16 +144,15 @@ actcapi_listen_req(act2000_card *card)
 
 	for (i = 0; i < ACT2000_BCH; i++)
 		eazmask |= card->bch[i].eazmask;
-	ACTCAPI_MKHDR(9, 0x05, 0x00);
-	if (!skb) {
-		printk(KERN_WARNING "actcapi: alloc_skb failed\n");
+	skb = actcapi_mkhdr(card, &m, 9, 0x05, 0x00);
+	if (!skb)
 		return -ENOMEM;
-	}
+
 	m->msg.listen_req.controller = 0;
 	m->msg.listen_req.infomask = 0x3f; /* All information */
 	m->msg.listen_req.eazmask = eazmask;
 	m->msg.listen_req.simask = (eazmask) ? 0x86 : 0; /* All SI's  */
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 	return 0;
 }
 
@@ -157,12 +163,12 @@ actcapi_connect_req(act2000_card *card, act2000_chan *chan, char *phone,
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR((11 + strlen(phone)), 0x02, 0x00);
+	skb = actcapi_mkhdr(card, &m, 11 + strlen(phone), 0x02, 0x00);
 	if (!skb) {
-		printk(KERN_WARNING "actcapi: alloc_skb failed\n");
 		chan->fsm_state = ACT2000_STATE_NULL;
 		return -ENOMEM;
 	}
+
 	m->msg.connect_req.controller = 0;
 	m->msg.connect_req.bchan = 0x83;
 	m->msg.connect_req.infomask = 0x3f;
@@ -173,7 +179,7 @@ actcapi_connect_req(act2000_card *card, act2000_chan *chan, char *phone,
 	m->msg.connect_req.addr.tnp = 0x81;
 	memcpy(m->msg.connect_req.addr.num, phone, strlen(phone));
 	chan->callref = m->hdr.msgnum;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 	return 0;
 }
 
@@ -183,14 +189,16 @@ actcapi_connect_b3_req(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(17, 0x82, 0x00);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 17, 0x82, 0x00);
+	if (!skb)
+		return;
+
 	m->msg.connect_b3_req.plci = chan->plci;
 	memset(&m->msg.connect_b3_req.ncpi, 0,
 	       sizeof(m->msg.connect_b3_req.ncpi));
 	m->msg.connect_b3_req.ncpi.len = 13;
 	m->msg.connect_b3_req.ncpi.modulo = 8;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 /*
@@ -202,15 +210,14 @@ actcapi_manufacturer_req_net(act2000_card *card)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(5, 0xff, 0x00);
-	if (!skb) {
-		printk(KERN_WARNING "actcapi: alloc_skb failed\n");
+	skb = actcapi_mkhdr(card, &m, 5, 0xff, 0x00);
+	if (!skb)
 		return -ENOMEM;
-	}
+
 	m->msg.manufacturer_req_net.manuf_msg = 0x11;
 	m->msg.manufacturer_req_net.controller = 1;
 	m->msg.manufacturer_req_net.nettype = (card->ptype == ISDN_PTYPE_EURO) ? 1 : 0;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 	printk(KERN_INFO "act2000 %s: D-channel protocol now %s\n",
 	       card->interface.id, (card->ptype == ISDN_PTYPE_EURO) ? "euro" : "1tr6");
 	card->interface.features &=
@@ -230,16 +237,14 @@ actcapi_manufacturer_req_v42(act2000_card *card, ulong arg)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(8, 0xff, 0x00);
-	if (!skb) {
-
-		printk(KERN_WARNING "actcapi: alloc_skb failed\n");
+	skb = actcapi_mkhdr(card, &m, 8, 0xff, 0x00);
+	if (!skb)
 		return -ENOMEM;
-	}
+
 	m->msg.manufacturer_req_v42.manuf_msg = 0x10;
 	m->msg.manufacturer_req_v42.controller = 0;
 	m->msg.manufacturer_req_v42.v42control = (arg ? 1 : 0);
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 	return 0;
 }
 #endif  /*  0  */
@@ -253,15 +258,13 @@ actcapi_manufacturer_req_errh(act2000_card *card)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(4, 0xff, 0x00);
-	if (!skb) {
-
-		printk(KERN_WARNING "actcapi: alloc_skb failed\n");
+	skb = actcapi_mkhdr(card, &m, 4, 0xff, 0x00);
+	if (!skb)
 		return -ENOMEM;
-	}
+
 	m->msg.manufacturer_req_err.manuf_msg = 0x03;
 	m->msg.manufacturer_req_err.controller = 0;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 	return 0;
 }
 
@@ -281,17 +284,16 @@ actcapi_manufacturer_req_msn(act2000_card *card)
 
 		len = strlen(p->msn);
 		for (i = 0; i < 2; i++) {
-			ACTCAPI_MKHDR(6 + len, 0xff, 0x00);
-			if (!skb) {
-				printk(KERN_WARNING "actcapi: alloc_skb failed\n");
+			skb = actcapi_mkhdr(card, &m, 6 + len, 0xff, 0x00);
+			if (!skb)
 				return -ENOMEM;
-			}
+
 			m->msg.manufacturer_req_msn.manuf_msg = 0x13 + i;
 			m->msg.manufacturer_req_msn.controller = 0;
 			m->msg.manufacturer_req_msn.msnmap.eaz = p->eaz;
 			m->msg.manufacturer_req_msn.msnmap.len = len;
 			memcpy(m->msg.manufacturer_req_msn.msnmap.msn, p->msn, len);
-			ACTCAPI_QUEUE_TX;
+			actcapi_queue_tx(card, skb);
 		}
 		p = p->next;
 	}
@@ -304,8 +306,10 @@ actcapi_select_b2_protocol_req(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(10, 0x40, 0x00);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 10, 0x40, 0x00);
+	if (!skb)
+		return;
+
 	m->msg.select_b2_protocol_req.plci = chan->plci;
 	memset(&m->msg.select_b2_protocol_req.dlpd, 0,
 	       sizeof(m->msg.select_b2_protocol_req.dlpd));
@@ -330,7 +334,7 @@ actcapi_select_b2_protocol_req(act2000_card *card, act2000_chan *chan)
 		m->msg.select_b2_protocol_req.dlpd.modulo = 8;
 		break;
 	}
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -339,8 +343,10 @@ actcapi_select_b3_protocol_req(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(17, 0x80, 0x00);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 17, 0x80, 0x00);
+	if (!skb)
+		return;
+
 	m->msg.select_b3_protocol_req.plci = chan->plci;
 	memset(&m->msg.select_b3_protocol_req.ncpd, 0,
 	       sizeof(m->msg.select_b3_protocol_req.ncpd));
@@ -351,7 +357,7 @@ actcapi_select_b3_protocol_req(act2000_card *card, act2000_chan *chan)
 		m->msg.select_b3_protocol_req.ncpd.modulo = 8;
 		break;
 	}
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -360,10 +366,12 @@ actcapi_listen_b3_req(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(2, 0x81, 0x00);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 2, 0x81, 0x00);
+	if (!skb)
+		return;
+
 	m->msg.listen_b3_req.plci = chan->plci;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -372,11 +380,13 @@ actcapi_disconnect_req(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(3, 0x04, 0x00);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 3, 0x04, 0x00);
+	if (!skb)
+		return;
+
 	m->msg.disconnect_req.plci = chan->plci;
 	m->msg.disconnect_req.cause = 0;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 void
@@ -385,15 +395,17 @@ actcapi_disconnect_b3_req(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(17, 0x84, 0x00);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 17, 0x84, 0x00);
+	if (!skb)
+		return;
+
 	m->msg.disconnect_b3_req.ncci = chan->ncci;
 	memset(&m->msg.disconnect_b3_req.ncpi, 0,
 	       sizeof(m->msg.disconnect_b3_req.ncpi));
 	m->msg.disconnect_b3_req.ncpi.len = 13;
 	m->msg.disconnect_b3_req.ncpi.modulo = 8;
 	chan->fsm_state = ACT2000_STATE_BHWAIT;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 void
@@ -402,8 +414,10 @@ actcapi_connect_resp(act2000_card *card, act2000_chan *chan, __u8 cause)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(3, 0x02, 0x03);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 3, 0x02, 0x03);
+	if (!skb)
+		return;
+
 	m->msg.connect_resp.plci = chan->plci;
 	m->msg.connect_resp.rejectcause = cause;
 	if (cause) {
@@ -411,7 +425,7 @@ actcapi_connect_resp(act2000_card *card, act2000_chan *chan, __u8 cause)
 		chan->plci = 0x8000;
 	} else
 		chan->fsm_state = ACT2000_STATE_IWAIT;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -420,12 +434,14 @@ actcapi_connect_active_resp(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(2, 0x03, 0x03);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 2, 0x03, 0x03);
+	if (!skb)
+		return;
+
 	m->msg.connect_resp.plci = chan->plci;
 	if (chan->fsm_state == ACT2000_STATE_IWAIT)
 		chan->fsm_state = ACT2000_STATE_IBWAIT;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -434,8 +450,10 @@ actcapi_connect_b3_resp(act2000_card *card, act2000_chan *chan, __u8 rejectcause
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR((rejectcause ? 3 : 17), 0x82, 0x03);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, rejectcause ? 3 : 17, 0x82, 0x03);
+	if (!skb)
+		return;
+
 	m->msg.connect_b3_resp.ncci = chan->ncci;
 	m->msg.connect_b3_resp.rejectcause = rejectcause;
 	if (!rejectcause) {
@@ -445,7 +463,7 @@ actcapi_connect_b3_resp(act2000_card *card, act2000_chan *chan, __u8 rejectcause
 		m->msg.connect_b3_resp.ncpi.modulo = 8;
 		chan->fsm_state = ACT2000_STATE_BWAIT;
 	}
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -454,11 +472,13 @@ actcapi_connect_b3_active_resp(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(2, 0x83, 0x03);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 2, 0x83, 0x03);
+	if (!skb)
+		return;
+
 	m->msg.connect_b3_active_resp.ncci = chan->ncci;
 	chan->fsm_state = ACT2000_STATE_ACTIVE;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -467,10 +487,12 @@ actcapi_info_resp(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(2, 0x07, 0x03);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 2, 0x07, 0x03);
+	if (!skb)
+		return;
+
 	m->msg.info_resp.plci = chan->plci;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -479,12 +501,14 @@ actcapi_disconnect_b3_resp(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(2, 0x84, 0x03);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 2, 0x84, 0x03);
+	if (!skb)
+		return;
+
 	m->msg.disconnect_b3_resp.ncci = chan->ncci;
 	chan->ncci = 0x8000;
 	chan->queued = 0;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static void
@@ -493,11 +517,13 @@ actcapi_disconnect_resp(act2000_card *card, act2000_chan *chan)
 	actcapi_msg *m;
 	struct sk_buff *skb;
 
-	ACTCAPI_MKHDR(2, 0x04, 0x03);
-	ACTCAPI_CHKSKB;
+	skb = actcapi_mkhdr(card, &m, 2, 0x04, 0x03);
+	if (!skb)
+		return;
+
 	m->msg.disconnect_resp.plci = chan->plci;
 	chan->plci = 0x8000;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 }
 
 static int
@@ -575,7 +601,7 @@ actcapi_data_b3_ind(act2000_card *card, struct sk_buff *skb) {
 	msg->hdr.msgnum = actcapi_nextsmsg(card);
 	msg->msg.data_b3_resp.ncci = ncci;
 	msg->msg.data_b3_resp.blocknr = blocknr;
-	ACTCAPI_QUEUE_TX;
+	actcapi_queue_tx(card, skb);
 	return 1;
 }
 

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Paul Bolle]

[Adding Tilman.]

On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote:
> Does anyone still use these cards?

0) Good question.

1) None of the (two dozen) commits in drivers/isdn/act2000/ added since
v2.6.12 appear to be triggered complaints, suggestions, etc. of actual
users.

2) Broader picture: if I remember correctly there are now four different
flavors of ISDN in the kernel:
- really old: pre-i4l
- very old: i4l
- just old: CAPI
- not so old: mISDN

I could spend another 24 hours refining and relabeling these categories,
and matching the various drivers to these categories. But I'm pretty
sure none of the current categories contain all the drivers. And I'm
certain all current categories support one or more drivers that none of
the other categories do. So the current ISDN situation is a bit messy.

Tilman might be able to provide a clearer, and maybe less grumpy,
summary of the current situation.

3) Anyhow, this is an i4l card. i4l is deprecated since v2.6.22. And it
was obsolete before that!

4) Furthermore, it seems consumer grade ISDN is on the way out. Eg, my
ISP will disconnect my, let's call it, ADSL over ISDN connection in less
that two months because they can't be bothered anymore to support that
niche. 

5) So we could let the various flavors of ISDN limp along for another
few years. But maybe we should consider, say, removing i4l and pre i4l
and see who complains. That might be a rude thing to do. So perhaps the
various ISDN flavors should be left alone until ... what exactly?


Paul Bolle

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Bas Peters]

2015-02-07 21:43 GMT+01:00 Paul Bolle <pebolle@tiscali.nl>:
> [Adding Tilman.]
>
> On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote:
>> Does anyone still use these cards?
>
> 0) Good question.
>
> 1) None of the (two dozen) commits in drivers/isdn/act2000/ added since
> v2.6.12 appear to be triggered complaints, suggestions, etc. of actual
> users.
>
> 2) Broader picture: if I remember correctly there are now four different
> flavors of ISDN in the kernel:
> - really old: pre-i4l
> - very old: i4l
> - just old: CAPI
> - not so old: mISDN
>
> I could spend another 24 hours refining and relabeling these categories,
> and matching the various drivers to these categories. But I'm pretty
> sure none of the current categories contain all the drivers. And I'm
> certain all current categories support one or more drivers that none of
> the other categories do. So the current ISDN situation is a bit messy.
>
> Tilman might be able to provide a clearer, and maybe less grumpy,
> summary of the current situation.
>
> 3) Anyhow, this is an i4l card. i4l is deprecated since v2.6.22. And it
> was obsolete before that!
>
> 4) Furthermore, it seems consumer grade ISDN is on the way out. Eg, my
> ISP will disconnect my, let's call it, ADSL over ISDN connection in less
> that two months because they can't be bothered anymore to support that
> niche.
>
> 5) So we could let the various flavors of ISDN limp along for another
> few years. But maybe we should consider, say, removing i4l and pre i4l
> and see who complains. That might be a rude thing to do. So perhaps the
> various ISDN flavors should be left alone until ... what exactly?
>
>
> Paul Bolle
>

In that case I will drop this patchset. I thought it might have been
useful and a good way to learn
but I'm just wasting everyone's time with it if it's that deprecated.

Regards,

Bas

Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors

[Joe Perches]

On Sat, 2015-02-07 at 21:55 +0100, Bas Peters wrote:
> I thought it might have been
> useful and a good way to learn

Maybe pick a device you have (or maybe buy
something in the staging directory like a realtek
wireless device) and play with adding support for
something in it.

Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors)

[Tilman Schmidt]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 07.02.2015 um 21:43 schrieb Paul Bolle:
> On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote:
>> Does anyone still use these cards?
> 
> 0) Good question.

I very much doubt it. It was an ISA card, not even PnP. I'd imagine
any systems of that kind would be retired by now.

> 2) Broader picture: if I remember correctly there are now four
> different flavors of ISDN in the kernel: - really old: pre-i4l

I don't think any traces of that are still present in current kernel
releases. It should all have been left behind on the switch to 2.6.

> - very old: i4l - just old: CAPI - not so old: mISDN

Those are the in-tree ones. To make matters worse, the Asterisk people
invented three more (Zaptel, DAHDI and vISDN) which are maintained
out-of-tree. Apparently they weren't satisfied with any the in-tree
ones and didn't feel like helping to improve one of them to match
their needs.

> [snip] So the current ISDN situation is a bit messy.

That's putting it mildly.

> Tilman might be able to provide a clearer, and maybe less grumpy, 
> summary of the current situation.

Don't know about either. ;-)

> [M]aybe we should consider, say, removing i4l and pre i4l and see
> who complains. That might be a rude thing to do. So perhaps the 
> various ISDN flavors should be left alone until ... what exactly?

I'd support that step. I don't think it'll hurt anyone because the
cards supported by i4l are mostly ISA cards anyway. The only exceptions
are the HiSax family which is now supported by mISDN, and the Hypercope
family which is supported by CAPI.

In the past we had Documentation/feature-removal-schedule.txt for
announcing removals like that but Linus shot that down on 2012-10-01
(commit 9c0ece069b32e8e122aea71aa47181c10eb85ba7) so I guess
somebody could just submit a patch series starting with

- --- a/drivers/isdn/Kconfig
+++ b/drivers/isdn/Kconfig
@@ -20,25 +20,6 @@ menuconfig ISDN

 if ISDN

- -menuconfig ISDN_I4L
- -       tristate "Old ISDN4Linux (deprecated)"
- -       depends on TTY
- -       ---help---
- -         This driver allows you to use an ISDN adapter for networking
- -         connections and as dialin/out device.  The isdn-tty's have a
built
- -         in AT-compatible modem emulator.  Network devices support
autodial,
- -         channel-bundling, callback and caller-authentication without
having
- -         a daemon running.  A reduced T.70 protocol is supported with
tty's
- -         suitable for German BTX.  On D-Channel, the protocols EDSS1
- -         (Euro-ISDN) and 1TR6 (German style) are supported.  See
- -         <file:Documentation/isdn/README> for more information.
- -
- -         ISDN support in the linux kernel is moving towards a new API,
- -         called CAPI (Common ISDN Application Programming Interface).
- -         Therefore the old ISDN4Linux layer will eventually become
obsolete.
- -         It is still available, though, for use with adapters that
are not
- -         supported by the new CAPI subsystem yet.
- -
 source "drivers/isdn/i4l/Kconfig"

 menuconfig ISDN_CAPI

and working its way from that to remove anything that's become
unreachable.

Shall I?

- -- 
Tilman Schmidt                              E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJU171KAAoJEFPuqx0v+F+qnqgH/3KCC6wnXWKOxfuWZ13lwqa2
GuFy+DMYZbXi5e5IHIYnTO9LvA7uzg4ryokP4iMWazhIpx5Cx0Riil59LJExE59p
jsLizpDeZ+z+wwrEZHimc8lYEFO70abvgKL5NdkF8luc+QPIJAmDyLA+8c6J0rKo
VcIb+vq3hW06L7FeKD3Y03NU1xZDtG6HxhnCQRV5b8Ve6sfeu4Oz/83yLyvvfNbK
mnRWbUisO2TmBWcTbZ0QB889iPBRrR1HD4TLA9WZ38fQs7YbuKNbz4jxMKDDSh2i
JHDK8tzn5P+73CPHkvRfELODn+V5uzUWh6QHJFD+4sMOKvNZfbxlEmt1GANmto0=
=PtE1
-----END PGP SIGNATURE-----

Re: Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors)

[Joe Perches]

On Sun, 2015-02-08 at 20:47 +0100, Tilman Schmidt wrote:
> Am 07.02.2015 um 21:43 schrieb Paul Bolle:
> > On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote:
> >> Does anyone still use these cards?
> > 0) Good question.
> I very much doubt it. It was an ISA card, not even PnP. I'd imagine
> any systems of that kind would be retired by now.
[]
> I guess
> somebody could just submit a patch series starting with:
> +++ b/drivers/isdn/Kconfig
> @@ -20,25 +20,6 @@ menuconfig ISDN
> 
>  if ISDN
> 
> -menuconfig ISDN_I4L
> -       tristate "Old ISDN4Linux (deprecated)"
> -       depends on TTY
[]
> Shall I?

I think removing code for completely unused,
no longer sold or supported devices could be
a good thing.

If someone pipes up saying "I use that" within
some shortish time-frame, it's easy to undo.

Re: Kill I4L?

[Karsten Keil]

Am 08.02.2015 um 20:47 schrieb Tilman Schmidt:
> Am 07.02.2015 um 21:43 schrieb Paul Bolle:
>> On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote:
>>> Does anyone still use these cards?
> 
>> 0) Good question.
> 
> I very much doubt it. It was an ISA card, not even PnP. I'd imagine
> any systems of that kind would be retired by now.
> 

Agreed in general, but I know people still running such ISA based
machines - mostly PC in industrial environments with ISDN uplinks for
service and maintenance.
And I4L is also used in some vending machines for card authorization so
far I know.
They usually using old kernels (<3.0) so they are not directly affected
if this will be removed.

>> 2) Broader picture: if I remember correctly there are now four
>> different flavors of ISDN in the kernel: - really old: pre-i4l
> 
> I don't think any traces of that are still present in current kernel
> releases. It should all have been left behind on the switch to 2.6.
> 

Yes.

>> - very old: i4l - just old: CAPI - not so old: mISDN
> 
> Those are the in-tree ones. To make matters worse, the Asterisk people
> invented three more (Zaptel, DAHDI and vISDN) which are maintained
> out-of-tree. Apparently they weren't satisfied with any the in-tree
> ones and didn't feel like helping to improve one of them to match
> their needs.
> 
>> [snip] So the current ISDN situation is a bit messy.
> 
> That's putting it mildly.
> 
>> Tilman might be able to provide a clearer, and maybe less grumpy, 
>> summary of the current situation.
> 
> Don't know about either. ;-)
> 
>> [M]aybe we should consider, say, removing i4l and pre i4l and see
>> who complains. That might be a rude thing to do. So perhaps the 
>> various ISDN flavors should be left alone until ... what exactly?
> 
> I'd support that step. I don't think it'll hurt anyone because the
> cards supported by i4l are mostly ISA cards anyway. The only exceptions
> are the HiSax family which is now supported by mISDN, and the Hypercope
> family which is supported by CAPI.
> 
> In the past we had Documentation/feature-removal-schedule.txt for
> announcing removals like that but Linus shot that down on 2012-10-01
> (commit 9c0ece069b32e8e122aea71aa47181c10eb85ba7) so I guess
> somebody could just submit a patch series starting with
> 
> --- a/drivers/isdn/Kconfig
> +++ b/drivers/isdn/Kconfig
> @@ -20,25 +20,6 @@ menuconfig ISDN
> 
>  if ISDN
> 
> -menuconfig ISDN_I4L
> -       tristate "Old ISDN4Linux (deprecated)"
> -       depends on TTY
> -       ---help---
> -         This driver allows you to use an ISDN adapter for networking
> -         connections and as dialin/out device.  The isdn-tty's have a
> built
> -         in AT-compatible modem emulator.  Network devices support
> autodial,
> -         channel-bundling, callback and caller-authentication without
> having
> -         a daemon running.  A reduced T.70 protocol is supported with
> tty's
> -         suitable for German BTX.  On D-Channel, the protocols EDSS1
> -         (Euro-ISDN) and 1TR6 (German style) are supported.  See
> -         <file:Documentation/isdn/README> for more information.
> -
> -         ISDN support in the linux kernel is moving towards a new API,
> -         called CAPI (Common ISDN Application Programming Interface).
> -         Therefore the old ISDN4Linux layer will eventually become
> obsolete.
> -         It is still available, though, for use with adapters that
> are not
> -         supported by the new CAPI subsystem yet.
> -
>  source "drivers/isdn/i4l/Kconfig"
> 
>  menuconfig ISDN_CAPI
> 
> and working its way from that to remove anything that's become
> unreachable.
> 
> Shall I?
> 

But I4L is still the default in some Distros, so we should allow a
warning period. But again, I'm fine with this to do it.

Karsten

Re: Kill I4L?

[Bas Peters]

2015-02-09 0:59 GMT+01:00 Karsten Keil <kkeil@linux-pingi.de>:
> Am 08.02.2015 um 20:47 schrieb Tilman Schmidt:
>> Am 07.02.2015 um 21:43 schrieb Paul Bolle:
>>> On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote:
>>>> Does anyone still use these cards?
>>
>>> 0) Good question.
>>
>> I very much doubt it. It was an ISA card, not even PnP. I'd imagine
>> any systems of that kind would be retired by now.
>>
>
> Agreed in general, but I know people still running such ISA based
> machines - mostly PC in industrial environments with ISDN uplinks for
> service and maintenance.
> And I4L is also used in some vending machines for card authorization so
> far I know.
> They usually using old kernels (<3.0) so they are not directly affected
> if this will be removed.
>
>>> 2) Broader picture: if I remember correctly there are now four
>>> different flavors of ISDN in the kernel: - really old: pre-i4l
>>
>> I don't think any traces of that are still present in current kernel
>> releases. It should all have been left behind on the switch to 2.6.
>>
>
> Yes.
>
>>> - very old: i4l - just old: CAPI - not so old: mISDN
>>
>> Those are the in-tree ones. To make matters worse, the Asterisk people
>> invented three more (Zaptel, DAHDI and vISDN) which are maintained
>> out-of-tree. Apparently they weren't satisfied with any the in-tree
>> ones and didn't feel like helping to improve one of them to match
>> their needs.
>>
>>> [snip] So the current ISDN situation is a bit messy.
>>
>> That's putting it mildly.
>>
>>> Tilman might be able to provide a clearer, and maybe less grumpy,
>>> summary of the current situation.
>>
>> Don't know about either. ;-)
>>
>>> [M]aybe we should consider, say, removing i4l and pre i4l and see
>>> who complains. That might be a rude thing to do. So perhaps the
>>> various ISDN flavors should be left alone until ... what exactly?
>>
>> I'd support that step. I don't think it'll hurt anyone because the
>> cards supported by i4l are mostly ISA cards anyway. The only exceptions
>> are the HiSax family which is now supported by mISDN, and the Hypercope
>> family which is supported by CAPI.
>>
>> In the past we had Documentation/feature-removal-schedule.txt for
>> announcing removals like that but Linus shot that down on 2012-10-01
>> (commit 9c0ece069b32e8e122aea71aa47181c10eb85ba7) so I guess
>> somebody could just submit a patch series starting with
>>
>> --- a/drivers/isdn/Kconfig
>> +++ b/drivers/isdn/Kconfig
>> @@ -20,25 +20,6 @@ menuconfig ISDN
>>
>>  if ISDN
>>
>> -menuconfig ISDN_I4L
>> -       tristate "Old ISDN4Linux (deprecated)"
>> -       depends on TTY
>> -       ---help---
>> -         This driver allows you to use an ISDN adapter for networking
>> -         connections and as dialin/out device.  The isdn-tty's have a
>> built
>> -         in AT-compatible modem emulator.  Network devices support
>> autodial,
>> -         channel-bundling, callback and caller-authentication without
>> having
>> -         a daemon running.  A reduced T.70 protocol is supported with
>> tty's
>> -         suitable for German BTX.  On D-Channel, the protocols EDSS1
>> -         (Euro-ISDN) and 1TR6 (German style) are supported.  See
>> -         <file:Documentation/isdn/README> for more information.
>> -
>> -         ISDN support in the linux kernel is moving towards a new API,
>> -         called CAPI (Common ISDN Application Programming Interface).
>> -         Therefore the old ISDN4Linux layer will eventually become
>> obsolete.
>> -         It is still available, though, for use with adapters that
>> are not
>> -         supported by the new CAPI subsystem yet.
>> -
>>  source "drivers/isdn/i4l/Kconfig"
>>
>>  menuconfig ISDN_CAPI
>>
>> and working its way from that to remove anything that's become
>> unreachable.
>>
>> Shall I?
>>
>
> But I4L is still the default in some Distros, so we should allow a
> warning period. But again, I'm fine with this to do it.

Is there any explicit reason why 'dead' drivers that might still have
some users ought to be removed?
Does it hurt anyone to leave the code in there, despite it barely
being used? We're not talking about
a particularly huge driver here, either.

>
> Karsten

Re: Kill I4L?

[Tilman Schmidt]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 09.02.2015 um 11:07 schrieb Bas Peters:
> 2015-02-09 0:59 GMT+01:00 Karsten Keil <kkeil@linux-pingi.de>:
>> Am 08.02.2015 um 20:47 schrieb Tilman Schmidt:
>>> Am 07.02.2015 um 21:43 schrieb Paul Bolle:

>>>> [M]aybe we should consider, say, removing i4l and pre i4l and
>>>> see who complains. That might be a rude thing to do. So
>>>> perhaps the various ISDN flavors should be left alone until
>>>> ... what exactly?
>>> 
>>> I'd support that step. I don't think it'll hurt anyone because
>>> the cards supported by i4l are mostly ISA cards anyway. The
>>> only exceptions are the HiSax family which is now supported by
>>> mISDN, and the Hypercope family which is supported by CAPI.
[...]
>> 
>> But I4L is still the default in some Distros, so we should allow
>> a warning period. But again, I'm fine with this to do it.
> 
> Is there any explicit reason why 'dead' drivers that might still
> have some users ought to be removed?

The reason is the maintenance load it produces. There's a continuous,
annoying trickle of patch  proposals, discussions, conflicts with
development in other, still actively maintained areas of the kernel,
and so on. The present discussion being a point in case.

> Does it hurt anyone to leave the code in there, despite it barely 
> being used?

Yes it does. Not much, but the pain is increasing over the years.
Every time someone tries to touch that code there's the problem
that no one can actually answer for it, much less test anything.
Theoretically a patch for a driver should not be accepted without
testing it on the actual hardware, but in the isdn tree that rule
has long been abandoned because nobody has the hardware and can do
the test. Consequently it isn't even clear whether all of it still
actually works.
It also hurts the few remaining Linux ISDN developers, distros and
users that Linux ISDN support is so fragmented. For example, the
Gigaset ISDN driver which I maintain can be built with either CAPI
or I4L support, so each time I touch it I have to build and test
two variants.

> We're not talking about a particularly huge driver here, either.

But one that's particularly difficult to maintain, without
providing any noticeable benefit in return.

- -- 
Tilman Schmidt                              E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJU2JHAAAoJEFPuqx0v+F+qn5EH/0iXrzTWChbu/0W8nDz4/qtC
FWQYbeIxTutzsEtVAhOYM7mz+9mqgaqNkVpmrz4lLg3FY4q2kzG1GjSihqP0GsT+
rLWJ+7gTnNxjNOk6OOZo+GaOjcvtVAro/2N5NXhHxTseumbH4I371a2rw0HBls97
iCPB2g6mJvNnsLjb612qcgsGahxMWVE/3q+6O1IKujPCTNQsJNaeqQMPT3YFJwq+
4YMs55RpVbpP5GPdRsaW/Zkwx8Se/4cK1MFaqX9xEePgZDUYMCPT2BPEa7E3yUwF
kjJU5LnBTKAjI8IzXDPTzznAyrMnH6IAjtJSmwpnyNintv2dtCK0VPhjhh0TA/M=
=d5Or
-----END PGP SIGNATURE-----

Re: Kill I4L?

[One Thousand Gnomes]

> The reason is the maintenance load it produces. There's a continuous,
> annoying trickle of patch  proposals, discussions, conflicts with
> development in other, still actively maintained areas of the kernel,
> and so on. The present discussion being a point in case.
> 
> > Does it hurt anyone to leave the code in there, despite it barely 
> > being used?
> 
> Yes it does. Not much, but the pain is increasing over the years.
> Every time someone tries to touch that code there's the problem
> that no one can actually answer for it, much less test anything.

The same has been happening with a lot of other code. For i2o I've
followed the pattern a few other drivers have used. I sent GregKH a patch
to move it into staging, and if nobody steps up then it will vanish in a
few releases.

> > We're not talking about a particularly huge driver here, either.
> 
> But one that's particularly difficult to maintain, without
> providing any noticeable benefit in return.

I'm also not sure a pretty, polished and untested driver is actually
better than someone who needs it going back to an old tree and a known
working driver to forward port.

Alan

Re: Kill I4L?

[Paul Bolle]

On Mon, 2015-02-09 at 00:59 +0100, Karsten Keil wrote:
> Am 08.02.2015 um 20:47 schrieb Tilman Schmidt:
> > Am 07.02.2015 um 21:43 schrieb Paul Bolle:
> >> 2) Broader picture: if I remember correctly there are now four
> >> different flavors of ISDN in the kernel: - really old: pre-i4l
> > 
> > I don't think any traces of that are still present in current kernel
> > releases. It should all have been left behind on the switch to 2.6. 
> 
> Yes.

I just refreshed my memory: drivers/isdn/hysdn predates I4L. It was
added in v2.3.45, while I4L was mainlined in v2.6.4. But, on the other
hand, one can optionally use HYSDN via "just old" CAPI.

(HYSDN looks a bit odd. By default "they register as ethernet card". I
guess this will only work if both ends of the ISDN connection have a
setup like that. But, who knows, maybe someone is still using HYSDN in
that way. www.hypercope.de serves "Sponsored Listings", by the way.)

> >> - very old: i4l - just old: CAPI - not so old: mISDN
> > 
> > Those are the in-tree ones. To make matters worse, the Asterisk people
> > invented three more (Zaptel, DAHDI and vISDN) which are maintained
> > out-of-tree. Apparently they weren't satisfied with any the in-tree
> > ones and didn't feel like helping to improve one of them to match
> > their needs.
> > 
> >> [snip] So the current ISDN situation is a bit messy.
> > 
> > That's putting it mildly.
> > 
> >> Tilman might be able to provide a clearer, and maybe less grumpy, 
> >> summary of the current situation.
> > 
> > Don't know about either. ;-)


Paul Bolle

[PATCH 3/6] drivers: isdn: act2000: module.c: remove assignments of variables in if conditions

[Bas Peters]

This patch removes all assignments of variables in if conditions in module.c.

Signed-off-by: Bas Peters <baspeters93@gmail.com>
---
 drivers/isdn/act2000/module.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
index c3a1b06..352916a 100644
--- a/drivers/isdn/act2000/module.c
+++ b/drivers/isdn/act2000/module.c
@@ -289,7 +289,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 			if (copy_from_user(tmp, arg,
 					   sizeof(tmp)))
 				return -EFAULT;
-			if ((ret = act2000_set_msn(card, tmp)))
+			ret = act2000_set_msn(card, tmp);
+			if (ret)
 				return ret;
 			if (card->flags & ACT2000_FLAGS_RUNNING)
 				return (actcapi_manufacturer_req_msn(card));
@@ -312,7 +313,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 	case ISDN_CMD_DIAL:
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		if (!(chan = find_channel(card, c->arg & 0x0f)))
+		chan = find_channel(card, c->arg & 0x0f);
+		if (!chan)
 			break;
 		spin_lock_irqsave(&card->lock, flags);
 		if (chan->fsm_state != ACT2000_STATE_NULL) {
@@ -341,7 +343,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 	case ISDN_CMD_ACCEPTD:
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		if (!(chan = find_channel(card, c->arg & 0x0f)))
+		chan = find_channel(card, c->arg & 0x0f);
+		if (!chan)
 			break;
 		if (chan->fsm_state == ACT2000_STATE_ICALL)
 			actcapi_select_b2_protocol_req(card, chan);
@@ -353,7 +356,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 	case ISDN_CMD_HANGUP:
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		if (!(chan = find_channel(card, c->arg & 0x0f)))
+		chan = find_channel(card, c->arg & 0x0f);
+		if (!chan)
 			break;
 		switch (chan->fsm_state) {
 		case ACT2000_STATE_ICALL:
@@ -368,7 +372,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 	case ISDN_CMD_SETEAZ:
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		if (!(chan = find_channel(card, c->arg & 0x0f)))
+		chan = find_channel(card, c->arg & 0x0f);
+		if (!chan)
 			break;
 		if (strlen(c->parm.num)) {
 			if (card->ptype == ISDN_PTYPE_EURO) {
@@ -388,7 +393,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 	case ISDN_CMD_CLREAZ:
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		if (!(chan = find_channel(card, c->arg & 0x0f)))
+		chan = find_channel(card, c->arg & 0x0f);
+		if (!chan)
 			break;
 		chan->eazmask = 0;
 		actcapi_listen_req(card);
@@ -396,7 +402,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 	case ISDN_CMD_SETL2:
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		if (!(chan = find_channel(card, c->arg & 0x0f)))
+		chan = find_channel(card, c->arg & 0x0f);
+		if (!chan)
 			break;
 		chan->l2prot = (c->arg >> 8);
 		return 0;
@@ -407,7 +414,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 			printk(KERN_WARNING "L3 protocol unknown\n");
 			return -1;
 		}
-		if (!(chan = find_channel(card, c->arg & 0x0f)))
+		chan = find_channel(card, c->arg & 0x0f);
+		if (!chan)
 			break;
 		chan->l3prot = (c->arg >> 8);
 		return 0;
@@ -424,7 +432,8 @@ act2000_sendbuf(act2000_card *card, int channel, int ack, struct sk_buff *skb)
 	act2000_chan *chan;
 	actcapi_msg *msg;
 
-	if (!(chan = find_channel(card, channel)))
+	chan = find_channel(card, channel);
+	if (!chan)
 		return -1;
 	if (chan->fsm_state != ACT2000_STATE_ACTIVE)
 		return -1;
@@ -573,7 +582,8 @@ act2000_alloccard(int bus, int port, int irq, char *id)
 {
 	int i;
 	act2000_card *card;
-	if (!(card = kzalloc(sizeof(act2000_card), GFP_KERNEL))) {
+	card = kzalloc(sizeof(act2000_card), GFP_KERNEL);
+	if (!card) {
 		printk(KERN_WARNING
 		       "act2000: (%s) Could not allocate card-struct.\n", id);
 		return;
-- 
2.1.0

[PATCH 4/6] drivers: isdn: act2000: module.c: remove NULL-initialization of static variable.

[Bas Peters]

GCC takes care of this for us, thus it is not needed and theoretically
only hoggs memory, allbeit only a bit.

Signed-off-by: Bas Peters <baspeters93@gmail.com>
---
 drivers/isdn/act2000/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
index 352916a..9359b36 100644
--- a/drivers/isdn/act2000/module.c
+++ b/drivers/isdn/act2000/module.c
@@ -28,7 +28,7 @@ static unsigned short act2000_isa_ports[] =
 static act2000_card *cards = (act2000_card *) NULL;
 
 /* Parameters to be set by insmod */
-static int   act_bus  =  0;
+static int   act_bus;
 static int   act_port = -1;  /* -1 = Autoprobe  */
 static int   act_irq  = -1;
 static char *act_id   = "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";
-- 
2.1.0

[PATCH 5/6] drivers: isdn: act2000: module.c: remove parenthesres around return values.

[Bas Peters]

return is not a function, therefore parentheses are not needed.

Signed-off-by: Bas Peters <baspeters93@gmail.com>
---
 drivers/isdn/act2000/module.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
index 9359b36..889ffcb 100644
--- a/drivers/isdn/act2000/module.c
+++ b/drivers/isdn/act2000/module.c
@@ -111,7 +111,7 @@ act2000_find_eaz(act2000_card *card, char eaz)
 
 	while (p) {
 		if (p->eaz == eaz)
-			return (p->msn);
+			return p->msn;
 		p = p->next;
 	}
 	return ("\0");
@@ -293,7 +293,7 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
 			if (ret)
 				return ret;
 			if (card->flags & ACT2000_FLAGS_RUNNING)
-				return (actcapi_manufacturer_req_msn(card));
+				return actcapi_manufacturer_req_msn(card);
 			return 0;
 		case ACT2000_IOCTL_ADDCARD:
 			if (copy_from_user(&cdef, arg,
@@ -520,7 +520,7 @@ if_command(isdn_ctrl *c)
 	act2000_card *card = act2000_findcard(c->driver);
 
 	if (card)
-		return (act2000_command(card, c));
+		return act2000_command(card, c);
 	printk(KERN_ERR
 	       "act2000: if_command %d called with invalid driverId %d!\n",
 	       c->command, c->driver);
@@ -535,7 +535,7 @@ if_writecmd(const u_char __user *buf, int len, int id, int channel)
 	if (card) {
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		return (len);
+		return len;
 	}
 	printk(KERN_ERR
 	       "act2000: if_writecmd called with invalid driverId!\n");
@@ -550,7 +550,7 @@ if_readstatus(u_char __user *buf, int len, int id, int channel)
 	if (card) {
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		return (act2000_readstatus(buf, len, card));
+		return act2000_readstatus(buf, len, card);
 	}
 	printk(KERN_ERR
 	       "act2000: if_readstatus called with invalid driverId!\n");
@@ -565,7 +565,7 @@ if_sendbuf(int id, int channel, int ack, struct sk_buff *skb)
 	if (card) {
 		if (!(card->flags & ACT2000_FLAGS_RUNNING))
 			return -ENODEV;
-		return (act2000_sendbuf(card, channel, ack, skb));
+		return act2000_sendbuf(card, channel, ack, skb);
 	}
 	printk(KERN_ERR
 	       "act2000: if_sendbuf called with invalid driverId!\n");
-- 
2.1.0

[PATCH 6/6] drivers: isdn: act2000: fix wrongly positioned brace.

[Bas Peters]

Trivial, but why not? :)

Signed-off-by: Bas Peters <baspeters93@gmail.com>
---
 drivers/isdn/act2000/module.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
index 889ffcb..9ba98ce 100644
--- a/drivers/isdn/act2000/module.c
+++ b/drivers/isdn/act2000/module.c
@@ -19,8 +19,7 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 
-static unsigned short act2000_isa_ports[] =
-{
+static unsigned short act2000_isa_ports[] = {
 	0x0200, 0x0240, 0x0280, 0x02c0, 0x0300, 0x0340, 0x0380,
 	0xcfe0, 0xcfa0, 0xcf60, 0xcf20, 0xcee0, 0xcea0, 0xce60,
 };
-- 
2.1.0