netdev - Re: [PATCH v3 net-next 04/10] net: dsa: tag_ksz: add tag handling for Microchip LAN937x

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: <20210723192347.ykgszwjh53phvpcn@skbuf>
Date:   Fri, 23 Jul 2021 22:23:47 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Prasanna Vengateshan <prasanna.vengateshan@...rochip.com>
Cc:     andrew@...n.ch, netdev@...r.kernel.org, robh+dt@...nel.org,
        UNGLinuxDriver@...rochip.com, Woojung.Huh@...rochip.com,
        hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
        kuba@...nel.org, linux-kernel@...r.kernel.org,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v3 net-next 04/10] net: dsa: tag_ksz: add tag handling
 for Microchip LAN937x

On Fri, Jul 23, 2021 at 11:01:02PM +0530, Prasanna Vengateshan wrote:
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -187,10 +187,66 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
>  DSA_TAG_DRIVER(ksz9893_netdev_ops);
>  MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893);
>  
> +/* For xmit, 2 bytes are added before FCS.
> + * ---------------------------------------------------------------------------
> + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> + * ---------------------------------------------------------------------------
> + * tag0 : represents tag override, lookup and valid
> + * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x80=port8)
> + *
> + * For rcv, 1 byte is added before FCS.
> + * ---------------------------------------------------------------------------
> + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
> + * ---------------------------------------------------------------------------
> + * tag0 : zero-based value represents port
> + *	  (eg, 0x00=port1, 0x02=port3, 0x07=port8)
> + */
> +#define LAN937X_EGRESS_TAG_LEN		2
> +
> +#define LAN937X_TAIL_TAG_BLOCKING_OVERRIDE	BIT(11)
> +#define LAN937X_TAIL_TAG_LOOKUP			BIT(12)
> +#define LAN937X_TAIL_TAG_VALID			BIT(13)
> +#define LAN937X_TAIL_TAG_PORT_MASK		7
> +
> +static struct sk_buff *lan937x_xmit(struct sk_buff *skb,
> +				    struct net_device *dev)
> +{
> +	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	const struct ethhdr *hdr = eth_hdr(skb);
> +	__be16 *tag;
> +	u16 val;

There was a recent patch on net.git to fix an issue with DSA master
drivers which set NETIF_F_HW_CSUM in dev->vlan_features, which DSA
inherits.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=37120f23ac8998c250573ea3247ff77426551f69
Until we fix the issue at the DSA layer, could you also apply that fix
here please?

> +
> +	tag = skb_put(skb, LAN937X_EGRESS_TAG_LEN);
> +
> +	val = BIT(dp->index);
> +
> +	if (is_link_local_ether_addr(hdr->h_dest))
> +		val |= LAN937X_TAIL_TAG_BLOCKING_OVERRIDE;
> +
> +	/* Tail tag valid bit - This bit should always be set by the CPU*/
> +	val |= LAN937X_TAIL_TAG_VALID;

Please add an extra space here between "CPU" and the comment ending
delimiter.

> +
> +	*tag = cpu_to_be16(val);

This probably works for the arch you are testing on, but it is better to
avoid unaligned accesses when the skb->len is odd:
https://www.kernel.org/doc/Documentation/unaligned-memory-access.txt

You can use put_unaligned_be16().

> +
> +	return skb;
> +}