netdev - Re: [PATCH 2/4] net: dsa: Add Lantiq / Intel GSWIP tag support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180725142021.GB13891@lunn.ch>
Date:   Wed, 25 Jul 2018 16:20:21 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Hauke Mehrtens <hauke@...ke-m.de>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        john@...ozen.org, linux-mips@...ux-mips.org, dev@...sin.me,
        hauke.mehrtens@...el.com
Subject: Re: [PATCH 2/4] net: dsa: Add Lantiq / Intel GSWIP tag support

On Sat, Jul 21, 2018 at 09:13:56PM +0200, Hauke Mehrtens wrote:
> This handles the tag added by the PMAC on the VRX200 SoC line.
> 
> The GSWIP uses internally a GSWIP special tag which is located after the
> Ethernet header. The PMAC which connects the GSWIP to the CPU converts
> this special tag used by the GSWIP into the PMAC special tag which is
> added in front of the Ethernet header.
> 
> This was tested with GSWIP 2.0 found in the VRX200 SoCs, other GSWIP
> versions use slightly different PMAC special tags
> 
> Signed-off-by: Hauke Mehrtens <hauke@...ke-m.de>

Hi Hauke

This looks good. A new minor nitpicks below.

> +#include <linux/bitops.h>
> +#include <linux/etherdevice.h>
> +#include <linux/skbuff.h>
> +#include <net/dsa.h>
> +
> +#include "dsa_priv.h"
> +
> +
> +#define GSWIP_TX_HEADER_LEN		4

Single newline is sufficient.

> +/* Byte 3 */
> +#define GSWIP_TX_CRCGEN_DIS		BIT(23)

BIT(23) in a byte is a bit odd.

> +#define GSWIP_TX_SLPID_SHIFT		0	/* source port ID */
> +#define  GSWIP_TX_SLPID_CPU		2
> +#define  GSWIP_TX_SLPID_APP1		3
> +#define  GSWIP_TX_SLPID_APP2		4
> +#define  GSWIP_TX_SLPID_APP3		5
> +#define  GSWIP_TX_SLPID_APP4		6
> +#define  GSWIP_TX_SLPID_APP5		7
> +
> +
> +#define GSWIP_RX_HEADER_LEN	8

Single newline is sufficient. Please fix them all, if there are more
of them.

> +
> +/* special tag in RX path header */
> +/* Byte 7 */
> +#define GSWIP_RX_SPPID_SHIFT		4
> +#define GSWIP_RX_SPPID_MASK		GENMASK(6, 4)
> +
> +static struct sk_buff *gswip_tag_rcv(struct sk_buff *skb,
> +				     struct net_device *dev,
> +				     struct packet_type *pt)
> +{
> +	int port;
> +	u8 *gswip_tag;
> +
> +	if (unlikely(!pskb_may_pull(skb, GSWIP_RX_HEADER_LEN)))
> +		return NULL;
> +
> +	gswip_tag = ((u8 *)skb->data) - ETH_HLEN;

The cast should not be needed, data already is an unsigned char.

> +	skb_pull_rcsum(skb, GSWIP_RX_HEADER_LEN);
> +
> +	/* Get source port information */
> +	port = (gswip_tag[7] & GSWIP_RX_SPPID_MASK) >> GSWIP_RX_SPPID_SHIFT;
> +	skb->dev = dsa_master_find_slave(dev, 0, port);
> +	if (!skb->dev)
> +		return NULL;
> +
> +	return skb;
> +}

  Andrew