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 <ha...@hauke-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

Reply via email to