Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

From: Alvin Šipraga
Date: Sun Aug 22 2021 - 19:11:34 EST

Hi Vladimir,

On 8/23/21 12:13 AM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 09:31:41PM +0200, Alvin Šipraga wrote:
>> From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
>>
>> This commit implements a basic version of the 8 byte tag protocol used
>> in the Realtek RTL8365MB-VC switch, which carries with it a protocol
>> version of 0x04.
>>
>> The implementation itself only handles the parsing of the EtherType
>> value and Realtek protocol version, together with the source or
>> destination port fields. The rest is left unimplemented for now.
>>
>> The tag format is described in a confidential document provided to my
>> employer - Bang & Olufsen a/s - by Realtek Semiconductor Corp.
>> Permission has been granted by Realtek to publish this driver based on
>> that material, together with an extract from the document describing the
>> tag format and its fields. It is hoped that this will help future
>> implementors who do not have access to the material but who wish to
>> extend the functionality of drivers for chips which use this protocol.
>>
>> Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
>> ---
>> include/net/dsa.h | 2 +
>> net/dsa/Kconfig | 6 ++
>> net/dsa/Makefile | 1 +
>> net/dsa/tag_rtl8_4.c | 178 +++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 187 insertions(+)
>> create mode 100644 net/dsa/tag_rtl8_4.c
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 0c2cba45fa79..6d8b5a11d99a 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -51,6 +51,7 @@ struct phylink_link_state;
>> #define DSA_TAG_PROTO_SEVILLE_VALUE 21
>> #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
>> #define DSA_TAG_PROTO_SJA1110_VALUE 23
>> +#define DSA_TAG_PROTO_RTL8_4_VALUE 24
>>
>> enum dsa_tag_protocol {
>> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
>> @@ -77,6 +78,7 @@ enum dsa_tag_protocol {
>> DSA_TAG_PROTO_OCELOT_8021Q = DSA_TAG_PROTO_OCELOT_8021Q_VALUE,
>> DSA_TAG_PROTO_SEVILLE = DSA_TAG_PROTO_SEVILLE_VALUE,
>> DSA_TAG_PROTO_SJA1110 = DSA_TAG_PROTO_SJA1110_VALUE,
>> + DSA_TAG_PROTO_RTL8_4 = DSA_TAG_PROTO_RTL8_4_VALUE,
>> };
>>
>> struct dsa_switch;
>> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
>> index 548285539752..470a2f3e8f75 100644
>> --- a/net/dsa/Kconfig
>> +++ b/net/dsa/Kconfig
>> @@ -99,6 +99,12 @@ config NET_DSA_TAG_RTL4_A
>> Realtek switches with 4 byte protocol A tags, sich as found in
>> the Realtek RTL8366RB.
>>
>> +config NET_DSA_TAG_RTL8_4
>> + tristate "Tag driver for Realtek 8 byte protocol 4 tags"
>> + help
>> + Say Y or M if you want to enable support for tagging frames for Realtek
>> + switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
>> +
>> config NET_DSA_TAG_OCELOT
>> tristate "Tag driver for Ocelot family of switches, using NPI port"
>> depends on MSCC_OCELOT_SWITCH_LIB || \
>> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
>> index 67ea009f242c..01282817e80e 100644
>> --- a/net/dsa/Makefile
>> +++ b/net/dsa/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
>> obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
>> obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
>> obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
>> +obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
>> obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
>> obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
>> obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
>> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
>> new file mode 100644
>> index 000000000000..1082bafb6a2e
>> --- /dev/null
>> +++ b/net/dsa/tag_rtl8_4.c
>> @@ -0,0 +1,178 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Handler for Realtek 8 byte switch tags
>> + *
>> + * Copyright (C) 2021 Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
>> + *
>> + * NOTE: Currently only supports protocol "4" found in the RTL8365MB, hence
>> + * named tag_rtl8_4.
>> + *
>> + * This "proprietary tag" header has the following format:
>> + *
>> + * -------------------------------------------
>> + * | MAC DA | MAC SA | 8 byte tag | Type | ...
>> + * -------------------------------------------
>> + * _______________/ \______________________________________
>> + * / \
>> + * 0 7|8 15
>> + * |-----------------------------------+-----------------------------------|---
>> + * | (16-bit) | ^
>> + * | Realtek EtherType [0x8899] | |
>> + * |-----------------------------------+-----------------------------------| 8
>> + * | (8-bit) | (8-bit) |
>> + * | Protocol [0x04] | REASON | b
>> + * |-----------------------------------+-----------------------------------| y
>> + * | (1) | (1) | (2) | (1) | (3) | (1) | (1) | (1) | (5) | t
>> + * | FID_EN | X | FID | PRI_EN | PRI | KEEP | X | LEARN_DIS | X | e
>> + * |-----------------------------------+-----------------------------------| s
>> + * | (1) | (15-bit) | |
>> + * | ALLOW | TX/RX | v
>> + * |-----------------------------------+-----------------------------------|---
>> + *
>> + * With the following field descriptions:
>> + *
>> + * field | description
>> + * ------------+-------------
>> + * Realtek | 0x8899: indicates that this is a proprietary Realtek tag;
>> + * EtherType | note that Realtek uses the same EtherType for
>> + * | other incompatible tag formats (e.g. tag_rtl4_a.c)
>> + * Protocol | 0x04: indicates that this tag conforms to this format
>> + * X | reserved
>> + * ------------+-------------
>> + * REASON | reason for forwarding packet to CPU
>> + * FID_EN | 1: packet has an FID
>> + * | 0: no FID
>> + * FID | FID of packet (if FID_EN=1)
>> + * PRI_EN | 1: force priority of packet
>> + * | 0: don't force priority
>> + * PRI | priority of packet (if PRI_EN=1)
>> + * KEEP | preserve packet VLAN tag format
>
> What does it mean to preserve packet VLAN tag format? Trying to
> understand if the sane thing is to clear or set this bit. Does it mean
> to strip the VLAN tag on egress if the VLAN is configured as
> egress-untagged on the port?

I suppose you mean "Does it mean _don't_ strip the VLAN tag on egress..."?

I'm not sure what the semantics of this KEEP are. When I configure the
ports to be egress-untagged, the packets leave the port untagged. When I
configure the ports without egress-untagged, the packets leave the port
tagged. This is with the code as you see it - so KEEP=0. If I am to
hazard a guess, maybe it overrides any port-based egress-untagged
setting. I will run some tests tomorrow.

>
>> + * LEARN_DIS | don't learn the source MAC address of the packet
>> + * ALLOW | 1: treat TX/RX field as an allowance port mask, meaning the
>> + * | packet may only be forwarded to ports specified in the
>> + * | mask
>> + * | 0: no allowance port mask, TX/RX field is the forwarding
>> + * | port mask
>> + * TX/RX | TX (switch->CPU): port number the packet was received on
>> + * | RX (CPU->switch): forwarding port mask (if ALLOW=0)
>> + * | allowance port mask (if ALLOW=1)
>> + */
>> +
>> +#include <linux/etherdevice.h>
>> +#include <linux/bits.h>
>> +
>> +#include "dsa_priv.h"
>> +
>> +#define RTL8_4_TAG_LEN 8
>> +#define RTL8_4_ETHERTYPE 0x8899
>> +/* 0x04 = RTL8365MB DSA protocol
>> + */
>> +#define RTL8_4_PROTOCOL_RTL8365MB 0x04
>> +
>> +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
>> + struct net_device *dev)
>> +{
>> + struct dsa_port *dp = dsa_slave_to_port(dev);
>> + __be16 *p;
>> + u8 *tag;
>> + u16 out;
>> +
>> + /* Pad out so that the (stripped) packet is at least 64 bytes long
>> + * (including FCS), otherwise the switch will drop the packet.
>> + * Then we need an additional 8 bytes for the Realtek tag.
>> + */
>> + if (__skb_put_padto(skb, ETH_ZLEN + RTL8_4_TAG_LEN, false))
>> + return NULL;
>> +
>> + skb_push(skb, RTL8_4_TAG_LEN);
>> +
>> + dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
>> + tag = dsa_etype_header_pos_tx(skb);
>> +
>> + /* Set Realtek EtherType */
>> + p = (__be16 *)tag;
>
> You would have much fewer (zero) type casts if "tag" was a "__be16 *" in
> the first place. Additionally, you would not need "p" and you could work
> with tag[0], tag[1], tag[2], tag[3].

Thanks, I'll revisit this in v2.

>
>> + *p = htons(RTL8_4_ETHERTYPE);
>> +
>> + /* Set Protocol; zero REASON */
>> + p = (__be16 *)(tag + 2);
>> + *p = htons(RTL8_4_PROTOCOL_RTL8365MB << 8);
>> +
>> + /* Zero FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
>
> Please set LEARN_DIS. DSA has better ways to control what needs to be
> learned and what doesn't. Packets injected into a standalone port
> surely shouldn't have their MAC SA learned. Grep for "tx_fwd_offload" in
> the kernel, see what it takes to transmit a packet with
> skb->offload_fwd_mark = true, and you can clear LEARN_DIS and set ALLOW
> then.

OK, I'll take a look.

>
>> + p = (__be16 *)(tag + 4);
>> + *p = 0;
>> +
>> + /* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
>> + p = (__be16 *)(tag + 6);
>> + out = BIT(dp->index);
>
> Set but unused variable.

Thought I scrubbed this but apparently not. Will address in v2.

>
>> + *p = htons(~(1 << 15) & BIT(dp->index));
>
> I am deeply confused by this line.
>
> ~(1 << 15) is GENMASK(14, 0)
> By AND-ing it with BIT(dp->index), what do you gain?

Deliberate verbosity for the human who was engaged in writing the
tagging driver to begin with, but obviously stupid. I'll remove.

>
>> +
>> + return skb;
>> +}
>> +
>> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
>> + struct net_device *dev)
>> +{
>> + __be16 *p;
>> + u16 etype;
>> + u8 proto;
>> + u8 *tag;
>> + u8 port;
>> + u16 tmp;
>> +
>> + if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
>> + return NULL;
>> +
>> + tag = dsa_etype_header_pos_rx(skb);
>> +
>> + /* Parse Realtek EtherType */
>> + p = (__be16 *)tag;
>
> Same comment about it being more productive for "tag" to be "__be16 *".

Thanks.

>
>> + etype = ntohs(*p);
>> + if (unlikely(etype != RTL8_4_ETHERTYPE)) {
>> + netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);
>> + return NULL;
>> + }
>> +
>> + /* Parse Protocol; ignore REASON */
>> + p = (__be16 *)(tag + 2);
>> + tmp = ntohs(*p);
>> + proto = tmp >> 8;
>> + if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) {
>> + netdev_dbg(dev, "unknown realtek protocol 0x%02x\n", proto);
>> + return NULL;
>> + }
>> +
>> + /* Ignore FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
>> + p = (__be16 *)(tag + 4);
>
> Delete then?

Deliberate verbosity again - but I figure any half-decent compiler will
optimize this out to begin with. I thought it serves as a perfectly fine
"add stuff here" notice together with the comment, but I can remove in v2.

>
>> +
>> + /* Ignore ALLOW; parse TX (switch->CPU) */
>> + p = (__be16 *)(tag + 6);
>> + tmp = ntohs(*p);
>> + port = tmp & 0xf; /* Port number is the LSB 4 bits */
>> +
>> + skb->dev = dsa_master_find_slave(dev, 0, port);
>> + if (!skb->dev) {
>> + netdev_dbg(dev, "could not find slave for port %d\n", port);
>> + return NULL;
>> + }
>> +
>> + /* Remove tag and recalculate checksum */
>> + skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
>> +
>> + dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
>> +
>> + skb->offload_fwd_mark = 1;
>
> At the very least, please use
>
> dsa_default_offload_fwd_mark(skb);
>
> which does the right thing when the port is not offloading the bridge.

Sure. Can you elaborate on what you mean by "at the very least"? Can it
be improved even further?

>
> Also tell us more about REASON and ALLOW. Is there a bit in the RX tag
> which denotes that the packet was forwarded only to the host?

As I wrote to Andrew, REASON is undocumented and I have not investigated
this field yet. I have addressed ALLOW upstairs in this email, but
suffice to say I am not sure.

>
>> +
>> + return skb;
>> +}
>> +
>> +static const struct dsa_device_ops rtl8_4_netdev_ops = {
>> + .name = "rtl8_4",
>> + .proto = DSA_TAG_PROTO_RTL8_4,
>> + .xmit = rtl8_4_tag_xmit,
>> + .rcv = rtl8_4_tag_rcv,
>> + .needed_headroom = RTL8_4_TAG_LEN,
>> +};
>> +module_dsa_tag_driver(rtl8_4_netdev_ops);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4);
>> --
>> 2.32.0
>>