Date:	Sat, 07 Feb 2015 11:19:21 -0800
From:	Joe Perches <joe@...ches.com>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:	Bas Peters <baspeters93@...il.com>, isdn@...ux-pingi.de,
	julia.lawall@...6.fr, davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch
 errors

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


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

Powered by blists - more mailing lists