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]

Date:   Sun, 29 Jul 2018 16:01:06 +0200
From:   Hauke Mehrtens <hauke@...ke-m.de>
To:     Andrew Lunn <andrew@...n.ch>
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 07/25/2018 04:20 PM, Andrew Lunn wrote:
> 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.

removed
> 
>> +/* Byte 3 */
>> +#define GSWIP_TX_CRCGEN_DIS		BIT(23)
> 
> BIT(23) in a byte is a bit odd.

OK, this should be BIT(7)
The ordering of these defines was also strange I fixed that.

> 
>> +#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.

ok

>> +
>> +/* 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.

OK, I removed that.

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