diff mbox series

[net-next] drivers/net/wan/hdlc_fr: Reduce indentation in pvc_xmit

Message ID 20201003173528.41404-1-xie.he.0141@gmail.com
State New
Headers show
Series [net-next] drivers/net/wan/hdlc_fr: Reduce indentation in pvc_xmit | expand

Commit Message

Xie He Oct. 3, 2020, 5:35 p.m. UTC 
1. Keep the code for the normal (non-error) flow at the lowest
indentation level. This reduces indentation and makes the code feels
more natural to read.

2. Use "goto drop" for all error handling. This reduces duplicate code.

3. Change "dev_kfree_skb" to "kfree_skb" in error handling code.
"kfree_skb" is the correct function to call when dropping an skb due to
an error. "dev_kfree_skb", which is an alias of "consume_skb", is for
dropping skbs normally (not due to an error).

Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 58 ++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

Comments

Stephen Hemminger Oct. 3, 2020, 6:09 p.m. UTC | #1 
On Sat,  3 Oct 2020 10:35:28 -0700
Xie He <xie.he.0141@gmail.com> wrote:

> +	if (dev->type == ARPHRD_ETHER) {

> +		int pad = ETH_ZLEN - skb->len;

> +

> +		if (pad > 0) { /* Pad the frame with zeros */

> +			int len = skb->len;

> +

> +			if (skb_tailroom(skb) < pad)

> +				if (pskb_expand_head(skb, 0, pad, GFP_ATOMIC))

> +					goto drop;

> +			skb_put(skb, pad);

> +			memset(skb->data + len, 0, pad);

>  		}

>  	}


This code snippet is basically an version of skb_pad().
Probably it is very old and pre-dates that.
Could the code use skb_pad?
Xie He Oct. 3, 2020, 9:27 p.m. UTC | #2 
On Sat, Oct 3, 2020 at 11:10 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>

> This code snippet is basically an version of skb_pad().

> Probably it is very old and pre-dates that.

> Could the code use skb_pad?


Oh! Yes! I looked at the skb_pad function and I think we can use it in
this code.

Since it doesn't do skb_put, I think we can first call skb_pad and
then call skb_put.

Thanks for your suggestion. I'll change this patch to make use of
skb_pad and re-submit.
diff mbox series

Patch

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 3a44dad87602..48aaf435da50 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -416,38 +416,40 @@  static netdev_tx_t pvc_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct pvc_device *pvc = dev->ml_priv;
 
-	if (pvc->state.active) {
-		if (dev->type == ARPHRD_ETHER) {
-			int pad = ETH_ZLEN - skb->len;
-			if (pad > 0) { /* Pad the frame with zeros */
-				int len = skb->len;
-				if (skb_tailroom(skb) < pad)
-					if (pskb_expand_head(skb, 0, pad,
-							     GFP_ATOMIC)) {
-						dev->stats.tx_dropped++;
-						dev_kfree_skb(skb);
-						return NETDEV_TX_OK;
-					}
-				skb_put(skb, pad);
-				memset(skb->data + len, 0, pad);
-			}
-		}
-		skb->dev = dev;
-		if (!fr_hard_header(&skb, pvc->dlci)) {
-			dev->stats.tx_bytes += skb->len;
-			dev->stats.tx_packets++;
-			if (pvc->state.fecn) /* TX Congestion counter */
-				dev->stats.tx_compressed++;
-			skb->dev = pvc->frad;
-			skb->protocol = htons(ETH_P_HDLC);
-			skb_reset_network_header(skb);
-			dev_queue_xmit(skb);
-			return NETDEV_TX_OK;
+	if (!pvc->state.active)
+		goto drop;
+
+	if (dev->type == ARPHRD_ETHER) {
+		int pad = ETH_ZLEN - skb->len;
+
+		if (pad > 0) { /* Pad the frame with zeros */
+			int len = skb->len;
+
+			if (skb_tailroom(skb) < pad)
+				if (pskb_expand_head(skb, 0, pad, GFP_ATOMIC))
+					goto drop;
+			skb_put(skb, pad);
+			memset(skb->data + len, 0, pad);
 		}
 	}
 
+	skb->dev = dev;
+	if (fr_hard_header(&skb, pvc->dlci))
+		goto drop;
+
+	dev->stats.tx_bytes += skb->len;
+	dev->stats.tx_packets++;
+	if (pvc->state.fecn) /* TX Congestion counter */
+		dev->stats.tx_compressed++;
+	skb->dev = pvc->frad;
+	skb->protocol = htons(ETH_P_HDLC);
+	skb_reset_network_header(skb);
+	dev_queue_xmit(skb);
+	return NETDEV_TX_OK;
+
+drop:
 	dev->stats.tx_dropped++;
-	dev_kfree_skb(skb);
+	kfree_skb(skb);
 	return NETDEV_TX_OK;
 }