Message-ID: <d65a10b4-a481-50d1-6b27-99fcc9d5f847@bang-olufsen.dk>
Date:   Tue, 12 Oct 2021 12:56:11 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <olteanv@...il.com>,
        Alvin Šipraga <alvin@...s.dk>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 4/6] net: dsa: tag_rtl8_4: add realtek 8 byte
 protocol 4 tag

On 10/12/21 2:50 PM, Vladimir Oltean wrote:
> On Tue, Oct 12, 2021 at 02:35:53PM +0200, Alvin Šipraga wrote:
>> From: Alvin Šipraga <alsi@...g-olufsen.dk>
>>
>> This commit implements a basic version of the 8 byte tag protocol used
>> in the Realtek RTL8365MB-VC unmanaged 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
>> company by Realtek Semiconductor Corp. Permission has been granted by
>> the vendor 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.
>>
>> In addition, two possible values of the REASON field are specified,
>> based on experiments on my end. Realtek does not specify what value this
>> field can take.
>>
>> Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
>> ---
> 
> Reviewed-by: Vladimir Oltean <olteanv@...il.com>
> 
>> RFC -> v1:
>>    - minor changes to the big comment at the top, including some
>>      empirical information about the REASON code
>>    - use dev_*_ratelimited() instead of netdev_*() for logging
>>    - use warning instead of debug messages
>>    - use ETH_P_REALTEK from if_ether.h
>>    - set LEARN_DIS on xmit
>>    - remove superfluous variables/expressions and use __b16 for tag
>>      variable
>>    - use new helper functions to insert/remove CPU tag
>>    - set offload_fwd_mark properly using helper function
>>
>>   include/net/dsa.h    |   2 +
>>   net/dsa/Kconfig      |   6 ++
>>   net/dsa/Makefile     |   1 +
>>   net/dsa/tag_rtl8_4.c | 166 +++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 175 insertions(+)
>>   create mode 100644 net/dsa/tag_rtl8_4.c
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index d784e76113b8..783060e851a9 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 6c7f79e45886..57fc52b17d55 100644
>> --- a/net/dsa/Kconfig
>> +++ b/net/dsa/Kconfig
>> @@ -130,6 +130,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_LAN9303
>>   	tristate "Tag driver for SMSC/Microchip LAN9303 family of switches"
>>   	help
>> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
>> index f78d537044db..9f75820e7c98 100644
>> --- a/net/dsa/Makefile
>> +++ b/net/dsa/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
>>   obj-$(CONFIG_NET_DSA_TAG_OCELOT_8021Q) += tag_ocelot_8021q.o
>>   obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.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_SJA1105) += tag_sja1105.o
>>   obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
>>   obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
>> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
>> new file mode 100644
>> index 000000000000..da22fd12b645
>> --- /dev/null
>> +++ b/net/dsa/tag_rtl8_4.c
>> @@ -0,0 +1,166 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Handler for Realtek 8 byte switch tags
>> + *
>> + * Copyright (C) 2021 Alvin Šipraga <alsi@...g-olufsen.dk>
>> + *
>> + * NOTE: Currently only supports protocol "4" found in the RTL8365MB, hence
>> + * named tag_rtl8_4.
>> + *
>> + * This 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
> 
> After our previous discussion I thought you said these bits were an EFID_EN and EFID?
> Or did you reconsider?

No, I did not reconsider - I stand by that observation. I actually sent 
an email to our contact at Realtek to get some clarification. But until 
I get a response (not guaranteed) or start actually using the field in 
the code, I prefer to reproduce their documentation verbatim.

> 
>> + *  |   (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
>> + *               | 0: packet was forwarded or flooded to CPU
>> + *               | 80: packet was trapped 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
>> + *    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/bits.h>
>> +#include <linux/etherdevice.h>
>> +
>> +#include "dsa_priv.h"
>> +
>> +#define RTL8_4_TAG_LEN			8
>> +/* 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 *tag;
>> +
>> +	/* Pad out so 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 (unlikely(__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 */
>> +	tag[0] = htons(ETH_P_REALTEK);
>> +
>> +	/* Set Protocol; zero REASON */
>> +	tag[1] = htons(RTL8_4_PROTOCOL_RTL8365MB << 8);
>> +
>> +	/* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */
>> +	tag[2] = htons(1 << 5);
>> +
>> +	/* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
>> +	tag[3] = htons(BIT(dp->index));
>> +
>> +	return skb;
>> +}
>> +
>> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
>> +				      struct net_device *dev)
>> +{
>> +	__be16 *tag;
>> +	u16 etype;
>> +	u8 proto;
>> +	u8 port;
>> +
>> +	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
>> +		return NULL;
>> +
>> +	tag = dsa_etype_header_pos_rx(skb);
>> +
>> +	/* Parse Realtek EtherType */
>> +	etype = ntohs(tag[0]);
>> +	if (unlikely(etype != ETH_P_REALTEK)) {
>> +		dev_warn_ratelimited(&dev->dev,
>> +				     "non-realtek ethertype 0x%04x\n", etype);
>> +		return NULL;
>> +	}
>> +
>> +	/* Parse Protocol */
>> +	proto = ntohs(tag[1]) >> 8;
>> +	if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) {
>> +		dev_warn_ratelimited(&dev->dev,
>> +				     "unknown realtek protocol 0x%02x\n",
>> +				     proto);
>> +		return NULL;
>> +	}
>> +
>> +	/* Parse TX (switch->CPU) */
>> +	port = ntohs(tag[3]) & 0xf; /* Port number is the LSB 4 bits */
>> +	skb->dev = dsa_master_find_slave(dev, 0, port);
>> +	if (!skb->dev) {
>> +		dev_warn_ratelimited(&dev->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);
>> +
>> +	dsa_default_offload_fwd_mark(skb);
>> +
>> +	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
>>

Powered by blists - more mailing lists