[PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Changli Gao]

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
v2: use asm/unaligned.h, thanks Harvey.
 drivers/net/ppp_async.c   |   10 +++++-----
 drivers/net/ppp_deflate.c |    9 ++++-----
 drivers/net/ppp_generic.c |    9 ++++-----
 drivers/net/ppp_mppe.c    |    7 +++----
 drivers/net/ppp_synctty.c |    3 ++-
 5 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ppp_async.c b/drivers/net/ppp_async.c
index 78d70a6..cbe1e13 100644
--- a/drivers/net/ppp_async.c
+++ b/drivers/net/ppp_async.c
@@ -32,6 +32,7 @@
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>
 #include <asm/uaccess.h>
 #include <asm/string.h>
 
@@ -542,7 +543,7 @@ ppp_async_encode(struct asyncppp *ap)
 	data = ap->tpkt->data;
 	count = ap->tpkt->len;
 	fcs = ap->tfcs;
-	proto = (data[0] << 8) + data[1];
+	proto = get_unaligned_be16(data);
 
 	/*
 	 * LCP packets with code values between 1 (configure-reqest)
@@ -963,7 +964,7 @@ static void async_lcp_peek(struct asyncppp *ap, unsigned char *data,
 	code = data[0];
 	if (code != CONFACK && code != CONFREQ)
 		return;
-	dlen = (data[2] << 8) + data[3];
+	dlen = get_unaligned_be16(data + 2);
 	if (len < dlen)
 		return;		/* packet got truncated or length is bogus */
 
@@ -997,15 +998,14 @@ static void async_lcp_peek(struct asyncppp *ap, unsigned char *data,
 	while (dlen >= 2 && dlen >= data[1] && data[1] >= 2) {
 		switch (data[0]) {
 		case LCP_MRU:
-			val = (data[2] << 8) + data[3];
+			val = get_unaligned_be16(data + 2);
 			if (inbound)
 				ap->mru = val;
 			else
 				ap->chan.mtu = val;
 			break;
 		case LCP_ASYNCMAP:
-			val = (data[2] << 24) + (data[3] << 16)
-				+ (data[4] << 8) + data[5];
+			val = get_unaligned_be32(data + 2);
 			if (inbound)
 				ap->raccm = val;
 			else
diff --git a/drivers/net/ppp_deflate.c b/drivers/net/ppp_deflate.c
index 695bc83..df3ce78 100644
--- a/drivers/net/ppp_deflate.c
+++ b/drivers/net/ppp_deflate.c
@@ -41,6 +41,7 @@
 #include <linux/ppp-comp.h>
 
 #include <linux/zlib.h>
+#include <asm/unaligned.h>
 
 /*
  * State for a Deflate (de)compressor.
@@ -232,11 +233,9 @@ static int z_compress(void *arg, unsigned char *rptr, unsigned char *obuf,
 	 */
 	wptr[0] = PPP_ADDRESS(rptr);
 	wptr[1] = PPP_CONTROL(rptr);
-	wptr[2] = PPP_COMP >> 8;
-	wptr[3] = PPP_COMP;
+	put_unaligned_be16(PPP_COMP, wptr + 2);
 	wptr += PPP_HDRLEN;
-	wptr[0] = state->seqno >> 8;
-	wptr[1] = state->seqno;
+	put_unaligned_be16(state->seqno, wptr);
 	wptr += DEFLATE_OVHD;
 	olen = PPP_HDRLEN + DEFLATE_OVHD;
 	state->strm.next_out = wptr;
@@ -451,7 +450,7 @@ static int z_decompress(void *arg, unsigned char *ibuf, int isize,
 	}
 
 	/* Check the sequence number. */
-	seq = (ibuf[PPP_HDRLEN] << 8) + ibuf[PPP_HDRLEN+1];
+	seq = get_unaligned_be16(ibuf + PPP_HDRLEN);
 	if (seq != (state->seqno & 0xffff)) {
 		if (state->debug)
 			printk(KERN_DEBUG "z_decompress%d: bad seq # %d, expected %d\n",
diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 6456484..0a81f0b 100644
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -46,6 +46,7 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>
 #include <net/slhc_vj.h>
 #include <asm/atomic.h>
 
@@ -210,7 +211,7 @@ struct ppp_net {
 };
 
 /* Get the PPP protocol number from a skb */
-#define PPP_PROTO(skb)	(((skb)->data[0] << 8) + (skb)->data[1])
+#define PPP_PROTO(skb)	get_unaligned_be16((skb)->data)
 
 /* We limit the length of ppp->file.rq to this (arbitrary) value */
 #define PPP_MAX_RQLEN	32
@@ -964,8 +965,7 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	pp = skb_push(skb, 2);
 	proto = npindex_to_proto[npi];
-	pp[0] = proto >> 8;
-	pp[1] = proto;
+	put_unaligned_be16(proto, pp);
 
 	netif_stop_queue(dev);
 	skb_queue_tail(&ppp->file.xq, skb);
@@ -1473,8 +1473,7 @@ static int ppp_mp_explode(struct ppp *ppp, struct sk_buff *skb)
 		q = skb_put(frag, flen + hdrlen);
 
 		/* make the MP header */
-		q[0] = PPP_MP >> 8;
-		q[1] = PPP_MP;
+		put_unaligned_be16(PPP_MP, q);
 		if (ppp->flags & SC_MP_XSHORTSEQ) {
 			q[2] = bits + ((ppp->nxseq >> 8) & 0xf);
 			q[3] = ppp->nxseq;
diff --git a/drivers/net/ppp_mppe.c b/drivers/net/ppp_mppe.c
index 6d1a1b8..44dab65 100644
--- a/drivers/net/ppp_mppe.c
+++ b/drivers/net/ppp_mppe.c
@@ -55,6 +55,7 @@
 #include <linux/ppp_defs.h>
 #include <linux/ppp-comp.h>
 #include <linux/scatterlist.h>
+#include <asm/unaligned.h>
 
 #include "ppp_mppe.h"
 
@@ -395,16 +396,14 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
 	 */
 	obuf[0] = PPP_ADDRESS(ibuf);
 	obuf[1] = PPP_CONTROL(ibuf);
-	obuf[2] = PPP_COMP >> 8;	/* isize + MPPE_OVHD + 1 */
-	obuf[3] = PPP_COMP;	/* isize + MPPE_OVHD + 2 */
+	put_unaligned_be16(PPP_COMP, obuf + 2);
 	obuf += PPP_HDRLEN;
 
 	state->ccount = (state->ccount + 1) % MPPE_CCOUNT_SPACE;
 	if (state->debug >= 7)
 		printk(KERN_DEBUG "mppe_compress[%d]: ccount %d\n", state->unit,
 		       state->ccount);
-	obuf[0] = state->ccount >> 8;
-	obuf[1] = state->ccount & 0xff;
+	put_unaligned_be16(state->ccount, obuf);
 
 	if (!state->stateful ||	/* stateless mode     */
 	    ((state->ccount & 0xff) == 0xff) ||	/* "flag" packet      */
diff --git a/drivers/net/ppp_synctty.c b/drivers/net/ppp_synctty.c
index 4c95ec3..fe86826 100644
--- a/drivers/net/ppp_synctty.c
+++ b/drivers/net/ppp_synctty.c
@@ -45,6 +45,7 @@
 #include <linux/completion.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>
 #include <asm/uaccess.h>
 
 #define PPP_VERSION	"2.4.2"
@@ -563,7 +564,7 @@ ppp_sync_txmunge(struct syncppp *ap, struct sk_buff *skb)
 	int islcp;
 
 	data  = skb->data;
-	proto = (data[0] << 8) + data[1];
+	proto = get_unaligned_be16(data);
 
 	/* LCP packets with codes between 1 (configure-request)
 	 * and 7 (code-reject) must be sent as though no options

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Harvey Harrison]

On Thu, Jan 6, 2011 at 3:37 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

FWIW:

Reviewed-by: Harvey Harrison <harvey.harrison@gmail.com>

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Paul Mackerras]

On Fri, Jan 07, 2011 at 07:37:36AM +0800, Changli Gao wrote:

> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

This patch description is inadequate.  It should tell us why you are
making this change.  Does it result in smaller and/or faster code, and
if so by how much on what sort of machine?  Do you think it makes the
code clearer?  (I don't.)  Or is there some other motivation for this?

Paul.

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Changli Gao]

On Fri, Jan 7, 2011 at 11:01 AM, Paul Mackerras <paulus@samba.org> wrote:
> On Fri, Jan 07, 2011 at 07:37:36AM +0800, Changli Gao wrote:
>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>
> This patch description is inadequate.  It should tell us why you are
> making this change.  Does it result in smaller and/or faster code, and
> if so by how much on what sort of machine?  Do you think it makes the
> code clearer?  (I don't.)  Or is there some other motivation for this?
>

Good designed APIs always make code clearer, smaller and faster. It is
obvious enough I think.

The names of the functions imply the endianness, like comments. On
some MIPS architectures which support unaligned load and store
instructions, the APIs result in smaller and faster code.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[David Miller]

From: Changli Gao <xiaosuo@gmail.com>
Date: Sat, 8 Jan 2011 08:43:01 +0800

> On Fri, Jan 7, 2011 at 11:01 AM, Paul Mackerras <paulus@samba.org> wrote:
>> On Fri, Jan 07, 2011 at 07:37:36AM +0800, Changli Gao wrote:
>>
>>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>>
>> This patch description is inadequate.  It should tell us why you are
>> making this change.  Does it result in smaller and/or faster code, and
>> if so by how much on what sort of machine?  Do you think it makes the
>> code clearer?  (I don't.)  Or is there some other motivation for this?
>>
> 
> Good designed APIs always make code clearer, smaller and faster. It is
> obvious enough I think.

I have to say that every time I go read the header parsing code in the
PPP driver, I absolutely regret it.

And Changli's patch fixes some of the readability problems.

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Paul Mackerras]

On Fri, Jan 07, 2011 at 05:15:34PM -0800, David Miller wrote:

> I have to say that every time I go read the header parsing code in the
> PPP driver, I absolutely regret it.
> 
> And Changli's patch fixes some of the readability problems.

It's up to you whether you merge the patch or not, but surely you
agree it needs more than a zero-line description?

Paul.

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[David Miller]

From: Paul Mackerras <paulus@samba.org>
Date: Sat, 8 Jan 2011 14:13:20 +1100

> On Fri, Jan 07, 2011 at 05:15:34PM -0800, David Miller wrote:
> 
>> I have to say that every time I go read the header parsing code in the
>> PPP driver, I absolutely regret it.
>> 
>> And Changli's patch fixes some of the readability problems.
> 
> It's up to you whether you merge the patch or not, but surely you
> agree it needs more than a zero-line description?

It's entire sufficient to me.

He de-open-coded {get,put}_unaligned_be{16,32}() and when open-coding
is eliminated in this way a commit message of "Use {helper function
foo}." is more than enough.

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Jarek Poplawski]

David Miller wrote:
> From: Changli Gao <xiaosuo@gmail.com>
> Date: Sat, 8 Jan 2011 08:43:01 +0800
> 
>> On Fri, Jan 7, 2011 at 11:01 AM, Paul Mackerras <paulus@samba.org> wrote:
>>> On Fri, Jan 07, 2011 at 07:37:36AM +0800, Changli Gao wrote:
>>>
>>>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>>>
>>> This patch description is inadequate.  It should tell us why you are
>>> making this change.  Does it result in smaller and/or faster code, and
>>> if so by how much on what sort of machine?  Do you think it makes the
>>> code clearer?  (I don't.)  Or is there some other motivation for this?
>>>
>>
>> Good designed APIs always make code clearer, smaller and faster. It is
>> obvious enough I think.
> 
> I have to say that every time I go read the header parsing code in the
> PPP driver, I absolutely regret it.
> 
> And Changli's patch fixes some of the readability problems.

Just for the record: I agree with Paul that current code is more readable.
This code still requires thinking about specific bytes and the patch mixes
it only with word access.

Jarek P.

> @@ -395,16 +396,14 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
>  	 */
>  	obuf[0] = PPP_ADDRESS(ibuf);
>  	obuf[1] = PPP_CONTROL(ibuf);
> -	obuf[2] = PPP_COMP >> 8;	/* isize + MPPE_OVHD + 1 */
> -	obuf[3] = PPP_COMP;	/* isize + MPPE_OVHD + 2 */
> +	put_unaligned_be16(PPP_COMP, obuf + 2);
>  	obuf += PPP_HDRLEN;

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Eric Dumazet]

Le samedi 08 janvier 2011 à 11:04 +0100, Jarek Poplawski a écrit :

> Just for the record: I agree with Paul that current code is more readable.
> This code still requires thinking about specific bytes and the patch mixes
> it only with word access.
> 
> Jarek P.
> 
> > @@ -395,16 +396,14 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
> >  	 */
> >  	obuf[0] = PPP_ADDRESS(ibuf);
> >  	obuf[1] = PPP_CONTROL(ibuf);
> > -	obuf[2] = PPP_COMP >> 8;	/* isize + MPPE_OVHD + 1 */
> > -	obuf[3] = PPP_COMP;	/* isize + MPPE_OVHD + 2 */
> > +	put_unaligned_be16(PPP_COMP, obuf + 2);
> >  	obuf += PPP_HDRLEN;

Compilers are stupid not generating optimal code, so we should help them
a bit.

Yes, I agree this is ugly Jarek and makes reading of this code a bit
more complex, but this is a move we cannot stop. Number of functions,
macros, etc... is exploding and we must follow the trend ;)

41 c6 44 24 02 00       movb   $0x0,0x2(%r12)
41 c6 44 24 03 fd       movb   $0xfd,0x3(%r12)

After patch :

66 41 c7 44 24 02 00 fd   movw   $0xfd00,0x2(%r12)

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[Jarek Poplawski]

Eric Dumazet wrote:
> Le samedi 08 janvier 2011 à 11:04 +0100, Jarek Poplawski a écrit :
> 
>> Just for the record: I agree with Paul that current code is more readable.
>> This code still requires thinking about specific bytes and the patch mixes
>> it only with word access.
>>
>> Jarek P.
>>
>>> @@ -395,16 +396,14 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
>>>  	 */
>>>  	obuf[0] = PPP_ADDRESS(ibuf);
>>>  	obuf[1] = PPP_CONTROL(ibuf);
>>> -	obuf[2] = PPP_COMP >> 8;	/* isize + MPPE_OVHD + 1 */
>>> -	obuf[3] = PPP_COMP;	/* isize + MPPE_OVHD + 2 */
>>> +	put_unaligned_be16(PPP_COMP, obuf + 2);
>>>  	obuf += PPP_HDRLEN;
> 
> Compilers are stupid not generating optimal code, so we should help them
> a bit.
> 
> Yes, I agree this is ugly Jarek and makes reading of this code a bit
> more complex, but this is a move we cannot stop. Number of functions,
> macros, etc... is exploding and we must follow the trend ;)
> 
> 41 c6 44 24 02 00       movb   $0x0,0x2(%r12)
> 41 c6 44 24 03 fd       movb   $0xfd,0x3(%r12)
> 
> After patch :
> 
> 66 41 c7 44 24 02 00 fd   movw   $0xfd00,0x2(%r12)

And that's why Paul wanted more justification, because readability
gain is questionable.

Jarek P.

Re: [PATCH v2] net: ppp: use {get,put}_unaligned_be{16,32}

[David Miller]

From: Changli Gao <xiaosuo@gmail.com>
Date: Fri,  7 Jan 2011 07:37:36 +0800

> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

Applied.