Re: [PATCH net-next] drivers/net/wan/hdlc_fr: Improve fr_rx and add support for any Ethertype

From: Jakub Kicinski <kuba@kernel.org>
To: Xie He <xie.he.0141@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Krzysztof Halasa <khc@pm.waw.pl>
Subject: Re: [PATCH net-next] drivers/net/wan/hdlc_fr: Improve fr_rx and add support for any Ethertype
Date: Sun, 18 Oct 2020 15:05:17 -0700	[thread overview]
Message-ID: <20201018150517.2f3dfb5c@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20201017051951.363514-1-xie.he.0141@gmail.com>

On Fri, 16 Oct 2020 22:19:51 -0700 Xie He wrote:
> 1. Change the fr_rx function to make this driver support any Ethertype
> when receiving. (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.
> 
> 2. Change the use of the "dev" variable in fr_rx. Originally we do
> "dev = something", and then at the end do "if (dev) skb->dev = dev".
> Now we do "if (something) skb->dev = something", then at the end do
> "dev = skb->dev".
> 
> This is to make the logic of our code consistent with eth_type_trans
> (which we call). The eth_type_trans function expects a non-NULL pointer
> as a parameter and assigns it directly to skb->dev.
> 
> 3. Change the initial skb->len check in fr_fx 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 to check its
> existence when we actually need it.
> 
> Also add an fh->ea2 check to the initial checks in fr_fx. 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.
> 
> 4. Use "goto rx_drop" whenever we need to drop a valid frame.

Whenever you make a list like that it's a strong indication that 
each of these should be a separate commit. That makes things easier 
to review.

We have already sent a pull request for 5.10 and therefore net-next 
is closed for new drivers, features, and code refactoring.

Please repost when net-next reopens after 5.10-rc1 is cut.

(http://vger.kernel.org/~davem/net-next.html will not be up to date 
 this time around, sorry about that).

RFC patches sent for review only are obviously welcome at any time.