[PATCH net-next v6 0/5] net: hdlc_fr: Improve fr_rx and add support for any Ethertype

[PATCH net-next v6 0/5] net: hdlc_fr: Improve fr_rx and add support for any Ethertype

[Xie He]

The main purpose of this series is the last patch. The previous 4 patches
are just code clean-ups so that the last patch will not make the code too
messy. The patches must be applied in sequence.

The receiving code of this driver doesn't support arbitrary Ethertype
values. It only recognizes a few known Ethertypes when receiving and drops
skbs with other Ethertypes.

However, the standard document RFC 2427 allows Frame Relay to support any
Ethertype values. This series adds support for this.

Change from v5:
Small fix to the commit messages.

Change from v4:
Drop the change related to stats.rx_dropped from the 1st patch.
Switch the 3rd and 4th patch.
Improve the commit message of the 4th patch by stating why only a 2-byte
address field is accepted.

Change from v3:
Split the last patch into 2 patches.
Improve the commit message of the 1st patch to explicitly state that the
stats.rx_dropped count is also increased after "goto rx_error".

Change from v2:
Small fix to the commit messages.

Change from v1:
Small fix to the commit messages.

Xie He (5):
  net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames
  net: hdlc_fr: Change the use of "dev" in fr_rx to make the code
    cleaner
  net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC
    devices
  net: hdlc_fr: Improve the initial checks when we receive an skb
  net: hdlc_fr: Add support for any Ethertype

 drivers/net/wan/hdlc_fr.c | 118 +++++++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 46 deletions(-)

-- 
2.27.0

[PATCH net-next v6 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Xie He]

When the fr_rx function drops a received frame (because the protocol type
is not supported, or because the PVC virtual device that corresponds to
the DLCI number and the protocol type doesn't exist), the function frees
the skb and returns.

The code for freeing the skb and returning is repeated several times, this
patch uses "goto rx_drop" to replace them so that the code looks cleaner.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 409e5a7ad8e2..4db0e01b96a9 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -904,8 +904,7 @@ static int fr_rx(struct sk_buff *skb)
 		netdev_info(frad, "No PVC for received frame's DLCI %d\n",
 			    dlci);
 #endif
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
 	if (pvc->state.fecn != fh->fecn) {
@@ -963,14 +962,12 @@ static int fr_rx(struct sk_buff *skb)
 		default:
 			netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n",
 				    oui, pid);
-			dev_kfree_skb_any(skb);
-			return NET_RX_DROP;
+			goto rx_drop;
 		}
 	} else {
 		netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n",
 			    data[3], skb->len);
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
 	if (dev) {
@@ -982,12 +979,12 @@ static int fr_rx(struct sk_buff *skb)
 		netif_rx(skb);
 		return NET_RX_SUCCESS;
 	} else {
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
- rx_error:
+rx_error:
 	frad->stats.rx_errors++; /* Mark error */
+rx_drop:
 	dev_kfree_skb_any(skb);
 	return NET_RX_DROP;
 }
-- 
2.27.0

[PATCH net-next v6 2/5] net: hdlc_fr: Change the use of "dev" in fr_rx to make the code cleaner

[Xie He]

The eth_type_trans function is called when we receive frames carrying
Ethernet frames. This function expects a non-NULL pointer as an argument,
and assigns it directly to skb->dev.

However, the code handling other types of frames first assigns the pointer
to "dev", and then at the end checks whether the value is NULL, and if it
is not NULL, assigns it to skb->dev.

The two flows are different. Mixing them in this function makes the code
messy. It's better that we convert the second flow to align with how
eth_type_trans does things.

So this patch changes the code to: first make sure the pointer is not
NULL, then assign it directly to skb->dev. "dev" is no longer needed until
the end where we use it to update stats.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 4db0e01b96a9..71ee9b60d91b 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -880,7 +880,7 @@ static int fr_rx(struct sk_buff *skb)
 	u8 *data = skb->data;
 	u16 dlci;
 	struct pvc_device *pvc;
-	struct net_device *dev = NULL;
+	struct net_device *dev;
 
 	if (skb->len <= 4 || fh->ea1 || data[2] != FR_UI)
 		goto rx_error;
@@ -930,13 +930,17 @@ static int fr_rx(struct sk_buff *skb)
 	}
 
 	if (data[3] == NLPID_IP) {
+		if (!pvc->main)
+			goto rx_drop;
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
-		dev = pvc->main;
+		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IP);
 
 	} else if (data[3] == NLPID_IPV6) {
+		if (!pvc->main)
+			goto rx_drop;
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
-		dev = pvc->main;
+		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IPV6);
 
 	} else if (skb->len > 10 && data[3] == FR_PAD &&
@@ -950,13 +954,16 @@ static int fr_rx(struct sk_buff *skb)
 		case ETH_P_IPX:
 		case ETH_P_IP:	/* a long variant */
 		case ETH_P_IPV6:
-			dev = pvc->main;
+			if (!pvc->main)
+				goto rx_drop;
+			skb->dev = pvc->main;
 			skb->protocol = htons(pid);
 			break;
 
 		case 0x80C20007: /* bridged Ethernet frame */
-			if ((dev = pvc->ether) != NULL)
-				skb->protocol = eth_type_trans(skb, dev);
+			if (!pvc->ether)
+				goto rx_drop;
+			skb->protocol = eth_type_trans(skb, pvc->ether);
 			break;
 
 		default:
@@ -970,17 +977,13 @@ static int fr_rx(struct sk_buff *skb)
 		goto rx_drop;
 	}
 
-	if (dev) {
-		dev->stats.rx_packets++; /* PVC traffic */
-		dev->stats.rx_bytes += skb->len;
-		if (pvc->state.becn)
-			dev->stats.rx_compressed++;
-		skb->dev = dev;
-		netif_rx(skb);
-		return NET_RX_SUCCESS;
-	} else {
-		goto rx_drop;
-	}
+	dev = skb->dev;
+	dev->stats.rx_packets++; /* PVC traffic */
+	dev->stats.rx_bytes += skb->len;
+	if (pvc->state.becn)
+		dev->stats.rx_compressed++;
+	netif_rx(skb);
+	return NET_RX_SUCCESS;
 
 rx_error:
 	frad->stats.rx_errors++; /* Mark error */
-- 
2.27.0

[PATCH net-next v6 3/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Xie He]

When an skb is received on a normal (non-Ethernet-emulating) PVC device,
call skb_reset_mac_header before we pass it to upper layers.

This is because normal PVC devices don't have header_ops, so any header we
have would not be visible to upper layer code when sending, so the header
shouldn't be visible to upper layer code when receiving, either.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Acked-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 71ee9b60d91b..eb83116aa9df 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -935,6 +935,7 @@ static int fr_rx(struct sk_buff *skb)
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
 		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IP);
+		skb_reset_mac_header(skb);
 
 	} else if (data[3] == NLPID_IPV6) {
 		if (!pvc->main)
@@ -942,6 +943,7 @@ static int fr_rx(struct sk_buff *skb)
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
 		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IPV6);
+		skb_reset_mac_header(skb);
 
 	} else if (skb->len > 10 && data[3] == FR_PAD &&
 		   data[4] == NLPID_SNAP && data[5] == FR_PAD) {
@@ -958,6 +960,7 @@ static int fr_rx(struct sk_buff *skb)
 				goto rx_drop;
 			skb->dev = pvc->main;
 			skb->protocol = htons(pid);
+			skb_reset_mac_header(skb);
 			break;
 
 		case 0x80C20007: /* bridged Ethernet frame */
-- 
2.27.0

[PATCH net-next v6 4/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Xie He]

1.
Change the skb->len check from "<= 4" to "< 4".
At first we only need to ensure a 4-byte header is present. We indeed
normally need the 5th byte, too, but it'd be more logical and cleaner
to check its existence when we actually need it.

2.
Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
the second address byte is the final address byte. We only support the
case where the address length is 2 bytes. If the address length is not
2 bytes, the control field and the protocol field would not be the 3rd
and 4th byte as we assume. (Say it is 3 bytes, then the control field
and the protocol field would be the 4th and 5th byte instead.)

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index eb83116aa9df..98444f1d8cc3 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -882,7 +882,7 @@ static int fr_rx(struct sk_buff *skb)
 	struct pvc_device *pvc;
 	struct net_device *dev;
 
-	if (skb->len <= 4 || fh->ea1 || data[2] != FR_UI)
+	if (skb->len < 4 || fh->ea1 || !fh->ea2 || data[2] != FR_UI)
 		goto rx_error;
 
 	dlci = q922_to_dlci(skb->data);
-- 
2.27.0

[PATCH net-next v6 5/5] net: hdlc_fr: Add support for any Ethertype

[Xie He]

Change the fr_rx function to make this driver support any Ethertype
when receiving skbs on normal (non-Ethernet-emulating) PVC devices.
(This driver is already able to handle any Ethertype when sending.)

Originally in the fr_rx function, the code that parses the long (10-byte)
header only recognizes a few Ethertype values and drops frames with other
Ethertype values. This patch replaces this code to make fr_rx support
any Ethertype. This patch also creates a new function fr_snap_parse as
part of the new code.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Acked-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 75 +++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 98444f1d8cc3..0720f5f92caa 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -871,6 +871,45 @@ static int fr_lmi_recv(struct net_device *dev, struct sk_buff *skb)
 	return 0;
 }
 
+static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc)
+{
+	/* OUI 00-00-00 indicates an Ethertype follows */
+	if (skb->data[0] == 0x00 &&
+	    skb->data[1] == 0x00 &&
+	    skb->data[2] == 0x00) {
+		if (!pvc->main)
+			return -1;
+		skb->dev = pvc->main;
+		skb->protocol = *(__be16 *)(skb->data + 3); /* Ethertype */
+		skb_pull(skb, 5);
+		skb_reset_mac_header(skb);
+		return 0;
+
+	/* OUI 00-80-C2 stands for the 802.1 organization */
+	} else if (skb->data[0] == 0x00 &&
+		   skb->data[1] == 0x80 &&
+		   skb->data[2] == 0xC2) {
+		/* PID 00-07 stands for Ethernet frames without FCS */
+		if (skb->data[3] == 0x00 &&
+		    skb->data[4] == 0x07) {
+			if (!pvc->ether)
+				return -1;
+			skb_pull(skb, 5);
+			if (skb->len < ETH_HLEN)
+				return -1;
+			skb->protocol = eth_type_trans(skb, pvc->ether);
+			return 0;
+
+		/* PID unsupported */
+		} else {
+			return -1;
+		}
+
+	/* OUI unsupported */
+	} else {
+		return -1;
+	}
+}
 
 static int fr_rx(struct sk_buff *skb)
 {
@@ -945,35 +984,19 @@ static int fr_rx(struct sk_buff *skb)
 		skb->protocol = htons(ETH_P_IPV6);
 		skb_reset_mac_header(skb);
 
-	} else if (skb->len > 10 && data[3] == FR_PAD &&
-		   data[4] == NLPID_SNAP && data[5] == FR_PAD) {
-		u16 oui = ntohs(*(__be16*)(data + 6));
-		u16 pid = ntohs(*(__be16*)(data + 8));
-		skb_pull(skb, 10);
-
-		switch ((((u32)oui) << 16) | pid) {
-		case ETH_P_ARP: /* routed frame with SNAP */
-		case ETH_P_IPX:
-		case ETH_P_IP:	/* a long variant */
-		case ETH_P_IPV6:
-			if (!pvc->main)
-				goto rx_drop;
-			skb->dev = pvc->main;
-			skb->protocol = htons(pid);
-			skb_reset_mac_header(skb);
-			break;
-
-		case 0x80C20007: /* bridged Ethernet frame */
-			if (!pvc->ether)
+	} else if (data[3] == FR_PAD) {
+		if (skb->len < 5)
+			goto rx_error;
+		if (data[4] == NLPID_SNAP) { /* A SNAP header follows */
+			skb_pull(skb, 5);
+			if (skb->len < 5) /* Incomplete SNAP header */
+				goto rx_error;
+			if (fr_snap_parse(skb, pvc))
 				goto rx_drop;
-			skb->protocol = eth_type_trans(skb, pvc->ether);
-			break;
-
-		default:
-			netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n",
-				    oui, pid);
+		} else {
 			goto rx_drop;
 		}
+
 	} else {
 		netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n",
 			    data[3], skb->len);
-- 
2.27.0

Re: [PATCH net-next v6 4/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Willem de Bruijn]

On Fri, Oct 30, 2020 at 8:49 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> 1.
> Change the skb->len check from "<= 4" to "< 4".
> At first we only need to ensure a 4-byte header is present. We indeed
> normally need the 5th byte, too, but it'd be more logical and cleaner
> to check its existence when we actually need it.
>
> 2.
> Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
> the second address byte is the final address byte. We only support the
> case where the address length is 2 bytes. If the address length is not
> 2 bytes, the control field and the protocol field would not be the 3rd
> and 4th byte as we assume. (Say it is 3 bytes, then the control field
> and the protocol field would be the 4th and 5th byte instead.)
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next v6 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Willem de Bruijn]

On Fri, Oct 30, 2020 at 8:50 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> When the fr_rx function drops a received frame (because the protocol type
> is not supported, or because the PVC virtual device that corresponds to
> the DLCI number and the protocol type doesn't exist), the function frees
> the skb and returns.
>
> The code for freeing the skb and returning is repeated several times, this
> patch uses "goto rx_drop" to replace them so that the code looks cleaner.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---
>  drivers/net/wan/hdlc_fr.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index 409e5a7ad8e2..4db0e01b96a9 100644
> --- a/drivers/net/wan/hdlc_fr.c
> +++ b/drivers/net/wan/hdlc_fr.c
> @@ -904,8 +904,7 @@ static int fr_rx(struct sk_buff *skb)
>                 netdev_info(frad, "No PVC for received frame's DLCI %d\n",
>                             dlci);
>  #endif
> -               dev_kfree_skb_any(skb);
> -               return NET_RX_DROP;
> +               goto rx_drop;
>         }
>
>         if (pvc->state.fecn != fh->fecn) {
> @@ -963,14 +962,12 @@ static int fr_rx(struct sk_buff *skb)
>                 default:
>                         netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n",
>                                     oui, pid);
> -                       dev_kfree_skb_any(skb);
> -                       return NET_RX_DROP;
> +                       goto rx_drop;
>                 }
>         } else {
>                 netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n",
>                             data[3], skb->len);
> -               dev_kfree_skb_any(skb);
> -               return NET_RX_DROP;
> +               goto rx_drop;
>         }
>
>         if (dev) {
> @@ -982,12 +979,12 @@ static int fr_rx(struct sk_buff *skb)
>                 netif_rx(skb);
>                 return NET_RX_SUCCESS;
>         } else {
> -               dev_kfree_skb_any(skb);
> -               return NET_RX_DROP;
> +               goto rx_drop;
>         }
>
> - rx_error:
> +rx_error:
>         frad->stats.rx_errors++; /* Mark error */
> +rx_drop:
>         dev_kfree_skb_any(skb);
>         return NET_RX_DROP;

I meant that I don't think errors should be double counted in rx_error
and rx_drop. It is fine to count drops as either.

Especially without that, I'm not sure this and the follow-on patch add
much value. Minor code cleanups complicate backports of fixes.

Re: [PATCH net-next v6 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Xie He]

On Sat, Oct 31, 2020 at 7:33 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> > - rx_error:
> > +rx_error:
> >         frad->stats.rx_errors++; /* Mark error */
> > +rx_drop:
> >         dev_kfree_skb_any(skb);
> >         return NET_RX_DROP;
>
> I meant that I don't think errors should be double counted in rx_error
> and rx_drop. It is fine to count drops as either.

OK. Can we do that in another patch? Because I feel this would make
the code a little bit more complex. Let's keep this patch as only a
simple clean-up.

> Especially without that, I'm not sure this and the follow-on patch add
> much value. Minor code cleanups complicate backports of fixes.

To me this is necessary, because I feel hard to do any development on
un-cleaned-up code. I really don't know how to add my code without
these clean-ups, and even if I managed to do that, I would not be
happy with my code.

Re: [PATCH net-next v6 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Xie He]

On Sat, Oct 31, 2020 at 8:18 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> > Especially without that, I'm not sure this and the follow-on patch add
> > much value. Minor code cleanups complicate backports of fixes.
>
> To me this is necessary, because I feel hard to do any development on
> un-cleaned-up code. I really don't know how to add my code without
> these clean-ups, and even if I managed to do that, I would not be
> happy with my code.

And always keeping the user interface and even the code unchanged
contradicts my motivation of contributing to the Linux kernel. All my
contributions are motivated by the hope to clean things up. I'm not an
actual user of any of the code I contribute. If we adhere to the
philosophy of not doing any clean-ups, my contributions would be
meaningless.

Re: [PATCH net-next v6 4/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Xie He]

On Fri, Oct 30, 2020 at 5:49 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
> the second address byte is the final address byte. We only support the
> case where the address length is 2 bytes. If the address length is not
> 2 bytes, the control field and the protocol field would not be the 3rd
> and 4th byte as we assume. (Say it is 3 bytes, then the control field
> and the protocol field would be the 4th and 5th byte instead.)

No, I don't think adding this explanation (about why address lengths
other than 2 are not supported) is necessary. The code of this driver
completely doesn't support address lengths other than 2 for many
reasons, not only the reason above. The code for parsing and
generating the address field (q922_to_dlci and dlci_to_q922) supports
only 2-byte address fields. Also, the structure of the sending code of
this driver can only generate headers with a 2-byte address field.
Frame Relay networks usually have a fixed address length, that means a
certain network usually has only 1 fixed address length. If this
driver sends frames with 2-byte address fields, it shouldn't expect it
would receive frames with 3-byte or 4-byte address fields. There are
too many reasons, and I think just saying we only support 2-byte
address fields is completely enough. Explaining the reason as above
seems weird and inadequate to me, and also unnecessary to me.

Therefore, I will resend this patch series with this explanation
(about why address lengths other than 2 are not supported) removed.

Re: [PATCH net-next v6 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Willem de Bruijn]

On Sat, Oct 31, 2020 at 12:02 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Sat, Oct 31, 2020 at 8:18 AM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > > Especially without that, I'm not sure this and the follow-on patch add
> > > much value. Minor code cleanups complicate backports of fixes.
> >
> > To me this is necessary, because I feel hard to do any development on
> > un-cleaned-up code. I really don't know how to add my code without
> > these clean-ups, and even if I managed to do that, I would not be
> > happy with my code.

That is the reality of working in this space, I think. I have
frequently restructured code, fixed a bug and then worked backwards to
create a *minimal* bugfix that applies to the current code as well as
older stable branches.

Obviously this is more of a concern for stable fixes than for new
code. But we have to keep in mind that every code churn will make
future bug fixes harder to roll out to users. That is not to say that
churn should be avoided, just that we need to balance a change's
benefit against this cost.

> And always keeping the user interface and even the code unchanged
> contradicts my motivation of contributing to the Linux kernel. All my
> contributions are motivated by the hope to clean things up. I'm not an
> actual user of any of the code I contribute. If we adhere to the
> philosophy of not doing any clean-ups, my contributions would be
> meaningless.

There are cleanups and cleanups. Dead code removal and deduplication
of open coded logic, for instance, are very valuable. As is, for
instance, your work in making sense of hard_header_len.

Returning code in branches vs an error jump label seems more of a
personal preference, and to me does not pass the benefit/cost threshold.

FWIW, there is lots of code that I would jump at the opportunity to
restructure. Starting with skb_segment, probably.

Obviously, all this is just one opinion on the topic.

Re: [PATCH net-next v6 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Jakub Kicinski]

On Sat, 31 Oct 2020 15:47:28 -0400 Willem de Bruijn wrote:
> On Sat, Oct 31, 2020 at 12:02 PM Xie He <xie.he.0141@gmail.com> wrote:
> > On Sat, Oct 31, 2020 at 8:18 AM Xie He <xie.he.0141@gmail.com> wrote:  
> > > > Especially without that, I'm not sure this and the follow-on patch add
> > > > much value. Minor code cleanups complicate backports of fixes.  
> > >
> > > To me this is necessary, because I feel hard to do any development on
> > > un-cleaned-up code. I really don't know how to add my code without
> > > these clean-ups, and even if I managed to do that, I would not be
> > > happy with my code.  
> 
> That is the reality of working in this space, I think. I have
> frequently restructured code, fixed a bug and then worked backwards to
> create a *minimal* bugfix that applies to the current code as well as
> older stable branches.
> 
> Obviously this is more of a concern for stable fixes than for new
> code. But we have to keep in mind that every code churn will make
> future bug fixes harder to roll out to users. That is not to say that
> churn should be avoided, just that we need to balance a change's
> benefit against this cost.
> 
> > And always keeping the user interface and even the code unchanged
> > contradicts my motivation of contributing to the Linux kernel. All my
> > contributions are motivated by the hope to clean things up. I'm not an
> > actual user of any of the code I contribute. If we adhere to the
> > philosophy of not doing any clean-ups, my contributions would be
> > meaningless.  
> 
> There are cleanups and cleanups. Dead code removal and deduplication
> of open coded logic, for instance, are very valuable. As is, for
> instance, your work in making sense of hard_header_len.

Or removing the buggy uses of IFF_TX_SKB_SHARING, for that matter
(which at this point I agree we should just remove from ether_setup, 
and let people who care re-enable it).

> Returning code in branches vs an error jump label seems more of a
> personal preference, and to me does not pass the benefit/cost threshold.

I must agree.

Re: [PATCH net-next v6 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Xie He]

On Sat, Oct 31, 2020 at 12:48 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Returning code in branches vs an error jump label seems more of a
> personal preference, and to me does not pass the benefit/cost threshold.

This patch is necessary for the 2nd and 5th patch in this series,
because the 2nd and 5th patch would add a lot of places where we need
to "error out" and drop the frame. Without this patch, the 2nd and 5th
patch would add a lot of useless code.

The 2nd patch is also necessary for the 5th patch, because otherwise I
would not know how to produce the 5th patch. The logic is so
convoluted for me. And it seems to me that the simplest way for me
would make all code to follow the logic of eth_type_trans.

The patch series was actually a single patch previously:
http://patchwork.ozlabs.org/project/netdev/patch/20201017051951.363514-1-xie.he.0141@gmail.com/
I splitted it to make changes I do clearer. But really these patches
should be as a whole. It's really hard for me to do the 5th patch
without the 1st and 2nd patch.