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

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

[Xie He]

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

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

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

[Stephen Hemminger]

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?

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

[Xie He]

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.