LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors.
@ 2015-02-07 17:06 Bas Peters
  2015-02-07 17:06 ` [PATCH 1/6] drivers: isdn: act2000: act2000_isa.c: Fix " Bas Peters
                   ` (5 more replies)
  0 siblings, 6 replies; 23+ messages in thread
From: Bas Peters @ 2015-02-07 17:06 UTC (permalink / raw)
  To: isdn; +Cc: julia.lawall, davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/6] drivers: isdn: act2000: act2000_isa.c: Fix checkpatch errors
  2015-02-07 17:06 [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors Bas Peters
@ 2015-02-07 17:06 ` Bas Peters
  2015-02-07 17:06 ` [PATCH 2/6] drivers: isdn: act2000: capi.c: fix " Bas Peters
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Bas Peters @ 2015-02-07 17:06 UTC (permalink / raw)
  To: isdn; +Cc: julia.lawall, davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 17:06 [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors Bas Peters
  2015-02-07 17:06 ` [PATCH 1/6] drivers: isdn: act2000: act2000_isa.c: Fix " Bas Peters
@ 2015-02-07 17:06 ` Bas Peters
  2015-02-07 17:51   ` Sergei Shtylyov
  2015-02-07 17:06 ` [PATCH 3/6] drivers: isdn: act2000: module.c: remove assignments of variables in if conditions Bas Peters
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Bas Peters @ 2015-02-07 17:06 UTC (permalink / raw)
  To: isdn; +Cc: julia.lawall, davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 3/6] drivers: isdn: act2000: module.c: remove assignments of variables in if conditions
  2015-02-07 17:06 [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors Bas Peters
  2015-02-07 17:06 ` [PATCH 1/6] drivers: isdn: act2000: act2000_isa.c: Fix " Bas Peters
  2015-02-07 17:06 ` [PATCH 2/6] drivers: isdn: act2000: capi.c: fix " Bas Peters
@ 2015-02-07 17:06 ` Bas Peters
  2015-02-07 17:06 ` [PATCH 4/6] drivers: isdn: act2000: module.c: remove NULL-initialization of static variable Bas Peters
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Bas Peters @ 2015-02-07 17:06 UTC (permalink / raw)
  To: isdn; +Cc: julia.lawall, davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 4/6] drivers: isdn: act2000: module.c: remove NULL-initialization of static variable.
  2015-02-07 17:06 [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors Bas Peters
                   ` (2 preceding siblings ...)
  2015-02-07 17:06 ` [PATCH 3/6] drivers: isdn: act2000: module.c: remove assignments of variables in if conditions Bas Peters
@ 2015-02-07 17:06 ` Bas Peters
  2015-02-07 17:06 ` [PATCH 5/6] drivers: isdn: act2000: module.c: remove parenthesres around return values Bas Peters
  2015-02-07 17:06 ` [PATCH 6/6] drivers: isdn: act2000: fix wrongly positioned brace Bas Peters
  5 siblings, 0 replies; 23+ messages in thread
From: Bas Peters @ 2015-02-07 17:06 UTC (permalink / raw)
  To: isdn; +Cc: julia.lawall, davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 5/6] drivers: isdn: act2000: module.c: remove parenthesres around return values.
  2015-02-07 17:06 [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors Bas Peters
                   ` (3 preceding siblings ...)
  2015-02-07 17:06 ` [PATCH 4/6] drivers: isdn: act2000: module.c: remove NULL-initialization of static variable Bas Peters
@ 2015-02-07 17:06 ` Bas Peters
  2015-02-07 17:06 ` [PATCH 6/6] drivers: isdn: act2000: fix wrongly positioned brace Bas Peters
  5 siblings, 0 replies; 23+ messages in thread
From: Bas Peters @ 2015-02-07 17:06 UTC (permalink / raw)
  To: isdn; +Cc: julia.lawall, davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 6/6] drivers: isdn: act2000: fix wrongly positioned brace.
  2015-02-07 17:06 [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors Bas Peters
                   ` (4 preceding siblings ...)
  2015-02-07 17:06 ` [PATCH 5/6] drivers: isdn: act2000: module.c: remove parenthesres around return values Bas Peters
@ 2015-02-07 17:06 ` Bas Peters
  5 siblings, 0 replies; 23+ messages in thread
From: Bas Peters @ 2015-02-07 17:06 UTC (permalink / raw)
  To: isdn; +Cc: julia.lawall, davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 17:06 ` [PATCH 2/6] drivers: isdn: act2000: capi.c: fix " Bas Peters
@ 2015-02-07 17:51   ` Sergei Shtylyov
  2015-02-07 18:02     ` Bas Peters
                       ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Sergei Shtylyov @ 2015-02-07 17:51 UTC (permalink / raw)
  To: Bas Peters, isdn; +Cc: julia.lawall, davem, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 17:51   ` Sergei Shtylyov
@ 2015-02-07 18:02     ` Bas Peters
  2015-02-07 18:02     ` Julia Lawall
  2015-02-07 19:19     ` Joe Perches
  2 siblings, 0 replies; 23+ messages in thread
From: Bas Peters @ 2015-02-07 18:02 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: isdn, julia.lawall, David Miller, netdev, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 17:51   ` Sergei Shtylyov
  2015-02-07 18:02     ` Bas Peters
@ 2015-02-07 18:02     ` Julia Lawall
  2015-02-07 18:08       ` Bas Peters
  2015-02-07 19:19     ` Joe Perches
  2 siblings, 1 reply; 23+ messages in thread
From: Julia Lawall @ 2015-02-07 18:02 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Bas Peters, isdn, julia.lawall, davem, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 18:02     ` Julia Lawall
@ 2015-02-07 18:08       ` Bas Peters
  2015-02-07 18:25         ` Sergei Shtylyov
  0 siblings, 1 reply; 23+ messages in thread
From: Bas Peters @ 2015-02-07 18:08 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Sergei Shtylyov, isdn, David Miller, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 18:08       ` Bas Peters
@ 2015-02-07 18:25         ` Sergei Shtylyov
  0 siblings, 0 replies; 23+ messages in thread
From: Sergei Shtylyov @ 2015-02-07 18:25 UTC (permalink / raw)
  To: Bas Peters, Julia Lawall; +Cc: isdn, David Miller, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 17:51   ` Sergei Shtylyov
  2015-02-07 18:02     ` Bas Peters
  2015-02-07 18:02     ` Julia Lawall
@ 2015-02-07 19:19     ` Joe Perches
  2015-02-07 20:43       ` Paul Bolle
  2 siblings, 1 reply; 23+ messages in thread
From: Joe Perches @ 2015-02-07 19:19 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Bas Peters, isdn, julia.lawall, davem, netdev, linux-kernel

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 19:19     ` Joe Perches
@ 2015-02-07 20:43       ` Paul Bolle
  2015-02-07 20:55         ` Bas Peters
  2015-02-08 19:47         ` Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors) Tilman Schmidt
  0 siblings, 2 replies; 23+ messages in thread
From: Paul Bolle @ 2015-02-07 20:43 UTC (permalink / raw)
  To: Joe Perches
  Cc: Tilman Schmidt, Sergei Shtylyov, Bas Peters, isdn, julia.lawall,
	davem, netdev, linux-kernel

[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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 20:43       ` Paul Bolle
@ 2015-02-07 20:55         ` Bas Peters
  2015-02-07 21:04           ` Joe Perches
  2015-02-08 19:47         ` Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors) Tilman Schmidt
  1 sibling, 1 reply; 23+ messages in thread
From: Bas Peters @ 2015-02-07 20:55 UTC (permalink / raw)
  To: Paul Bolle
  Cc: Joe Perches, Tilman Schmidt, Sergei Shtylyov, isdn, Julia Lawall,
	David Miller, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors
  2015-02-07 20:55         ` Bas Peters
@ 2015-02-07 21:04           ` Joe Perches
  0 siblings, 0 replies; 23+ messages in thread
From: Joe Perches @ 2015-02-07 21:04 UTC (permalink / raw)
  To: Bas Peters
  Cc: Paul Bolle, Tilman Schmidt, Sergei Shtylyov, isdn, Julia Lawall,
	David Miller, netdev, linux-kernel

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.




^ permalink raw reply	[flat|nested] 23+ messages in thread

* Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors)
  2015-02-07 20:43       ` Paul Bolle
  2015-02-07 20:55         ` Bas Peters
@ 2015-02-08 19:47         ` Tilman Schmidt
  2015-02-08 20:04           ` Joe Perches
  2015-02-08 23:59           ` Kill I4L? Karsten Keil
  1 sibling, 2 replies; 23+ messages in thread
From: Tilman Schmidt @ 2015-02-08 19:47 UTC (permalink / raw)
  To: Paul Bolle, Joe Perches
  Cc: Sergei Shtylyov, Bas Peters, isdn, julia.lawall, davem, netdev,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors)
  2015-02-08 19:47         ` Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors) Tilman Schmidt
@ 2015-02-08 20:04           ` Joe Perches
  2015-02-08 23:59           ` Kill I4L? Karsten Keil
  1 sibling, 0 replies; 23+ messages in thread
From: Joe Perches @ 2015-02-08 20:04 UTC (permalink / raw)
  To: Tilman Schmidt
  Cc: Paul Bolle, Sergei Shtylyov, Bas Peters, isdn, julia.lawall,
	davem, netdev, linux-kernel

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Kill I4L?
  2015-02-08 19:47         ` Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors) Tilman Schmidt
  2015-02-08 20:04           ` Joe Perches
@ 2015-02-08 23:59           ` Karsten Keil
  2015-02-09 10:07             ` Bas Peters
  2015-02-09 12:12             ` Paul Bolle
  1 sibling, 2 replies; 23+ messages in thread
From: Karsten Keil @ 2015-02-08 23:59 UTC (permalink / raw)
  To: Tilman Schmidt, Paul Bolle, Joe Perches
  Cc: Sergei Shtylyov, Bas Peters, isdn, julia.lawall, davem, netdev,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Kill I4L?
  2015-02-08 23:59           ` Kill I4L? Karsten Keil
@ 2015-02-09 10:07             ` Bas Peters
  2015-02-09 10:53               ` Tilman Schmidt
  2015-02-09 12:12             ` Paul Bolle
  1 sibling, 1 reply; 23+ messages in thread
From: Bas Peters @ 2015-02-09 10:07 UTC (permalink / raw)
  To: Karsten Keil
  Cc: Tilman Schmidt, Paul Bolle, Joe Perches, Sergei Shtylyov, isdn,
	Julia Lawall, David Miller, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Kill I4L?
  2015-02-09 10:07             ` Bas Peters
@ 2015-02-09 10:53               ` Tilman Schmidt
  2015-02-09 19:48                 ` One Thousand Gnomes
  0 siblings, 1 reply; 23+ messages in thread
From: Tilman Schmidt @ 2015-02-09 10:53 UTC (permalink / raw)
  To: Bas Peters, Karsten Keil
  Cc: Paul Bolle, Joe Perches, Sergei Shtylyov, isdn, Julia Lawall,
	David Miller, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Kill I4L?
  2015-02-08 23:59           ` Kill I4L? Karsten Keil
  2015-02-09 10:07             ` Bas Peters
@ 2015-02-09 12:12             ` Paul Bolle
  1 sibling, 0 replies; 23+ messages in thread
From: Paul Bolle @ 2015-02-09 12:12 UTC (permalink / raw)
  To: Karsten Keil
  Cc: Tilman Schmidt, Joe Perches, Sergei Shtylyov, Bas Peters, isdn,
	julia.lawall, davem, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Kill I4L?
  2015-02-09 10:53               ` Tilman Schmidt
@ 2015-02-09 19:48                 ` One Thousand Gnomes
  0 siblings, 0 replies; 23+ messages in thread
From: One Thousand Gnomes @ 2015-02-09 19:48 UTC (permalink / raw)
  To: Tilman Schmidt
  Cc: Bas Peters, Karsten Keil, Paul Bolle, Joe Perches,
	Sergei Shtylyov, isdn, Julia Lawall, David Miller, netdev,
	linux-kernel

> 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2015-02-09 19:49 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-07 17:06 [PATCH 0/6] drivers: isdn: act2000: fix a variety of checkpatch errors Bas Peters
2015-02-07 17:06 ` [PATCH 1/6] drivers: isdn: act2000: act2000_isa.c: Fix " Bas Peters
2015-02-07 17:06 ` [PATCH 2/6] drivers: isdn: act2000: capi.c: fix " Bas Peters
2015-02-07 17:51   ` Sergei Shtylyov
2015-02-07 18:02     ` Bas Peters
2015-02-07 18:02     ` Julia Lawall
2015-02-07 18:08       ` Bas Peters
2015-02-07 18:25         ` Sergei Shtylyov
2015-02-07 19:19     ` Joe Perches
2015-02-07 20:43       ` Paul Bolle
2015-02-07 20:55         ` Bas Peters
2015-02-07 21:04           ` Joe Perches
2015-02-08 19:47         ` Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors) Tilman Schmidt
2015-02-08 20:04           ` Joe Perches
2015-02-08 23:59           ` Kill I4L? Karsten Keil
2015-02-09 10:07             ` Bas Peters
2015-02-09 10:53               ` Tilman Schmidt
2015-02-09 19:48                 ` One Thousand Gnomes
2015-02-09 12:12             ` Paul Bolle
2015-02-07 17:06 ` [PATCH 3/6] drivers: isdn: act2000: module.c: remove assignments of variables in if conditions Bas Peters
2015-02-07 17:06 ` [PATCH 4/6] drivers: isdn: act2000: module.c: remove NULL-initialization of static variable Bas Peters
2015-02-07 17:06 ` [PATCH 5/6] drivers: isdn: act2000: module.c: remove parenthesres around return values Bas Peters
2015-02-07 17:06 ` [PATCH 6/6] drivers: isdn: act2000: fix wrongly positioned brace Bas Peters

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).