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