Re: [PATCH net-next v2 4/4] net: hdlc_fr: Add support for any Ethertype

[Willem de Bruijn]

On Wed, Oct 28, 2020 at 6:58 PM Xie He <xie.he.0141@xxxxxxxxx> wrote:
>
> 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.
>
> Also add skb_reset_mac_header before we pass an skb (received on normal
> PVC devices) to upper layers. Because we don't use header_ops for normal
> PVC devices, we should hide the header from upper layer code in this case.

This should probably be a separate commit

> Cc: Krzysztof Halasa <khc@xxxxxxxxx>
> Signed-off-by: Xie He <xie.he.0141@xxxxxxxxx>
> ---
>  drivers/net/wan/hdlc_fr.c | 76 ++++++++++++++++++++++++++-------------
>  1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index 3639c2bfb141..e95efc14bc97 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 */

Does it make sense to define a struct snap_hdr instead of manually
casting all these bytes?

> +               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) {


And macros or constant integers to self document these kinds of fields.