netdev - Re: [net-next v4 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags

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: <2ac97f29-bfc2-4674-9569-278bb4492676@lunn.ch>
Date: Mon, 18 Aug 2025 19:07:15 +0200
From: Andrew Lunn <andrew@...n.ch>
To: David Yang <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Simon Horman <horms@...nel.org>,
	Russell King <linux@...linux.org.uk>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [net-next v4 2/3] net: dsa: tag_yt921x: add support for
 Motorcomm YT921x tags

> +static struct sk_buff *
> +yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(netdev);
> +	unsigned int port = dp->index;
> +	struct dsa_port *partner;
> +	__be16 *tag;
> +	u16 tx;
> +
> +	skb_push(skb, YT921X_TAG_LEN);
> +	dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> +
> +	tag = dsa_etype_header_pos_tx(skb);
> +
> +	/* We might use yt921x_priv::tag_eth_p, but
> +	 * 1. CPU_TAG_TPID could be configured anyway;
> +	 * 2. Are you using the right chip?
> +	 */
> +	tag[0] = htons(ETH_P_YT921X);
> +	/* Service VLAN tag not used */
> +	tag[1] = 0;
> +	tag[2] = 0;
> +	tx = YT921X_TAG_PORT_EN | YT921X_TAG_TX_PORTn(port);
> +	if (dp->hsr_dev)
> +		dsa_hsr_foreach_port(partner, dp->ds, dp->hsr_dev)
> +			tx |= YT921X_TAG_TX_PORTn(partner->index);

As far as i remember, this was not in v1. When i spotting this in v2
that made me comment you should not add new features in revision of a
patch.

Does the current version of the DSA driver support hsr? Is this
useful? Maybe it would be better to add hsr support as a follow up
patch?

> +static struct sk_buff *
> +yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
> +{
> +	unsigned int port;
> +	__be16 *tag;
> +	u16 rx;
> +
> +	if (unlikely(!pskb_may_pull(skb, YT921X_TAG_LEN)))
> +		return NULL;
> +
> +	tag = (__be16 *)skb->data;
> +
> +	/* Locate which port this is coming from */
> +	rx = ntohs(tag[1]);
> +	if (unlikely((rx & YT921X_TAG_PORT_EN) == 0)) {
> +		netdev_err(netdev, "Unexpected rx tag 0x%04x\n", rx);
> +		return NULL;
> +	}
> +
> +	port = FIELD_GET(YT921X_TAG_RX_PORT_M, rx);
> +	skb->dev = dsa_conduit_find_user(netdev, 0, port);
> +	if (unlikely(!skb->dev)) {
> +		netdev_err(netdev, "Cannot locate rx port %u\n", port);
> +		return NULL;
> +	}

O.K. Stop. Think.

You changed the rate limiting to an unlimiting netdev_err().

What is the difference? Under what conditions would you want to use
rate limiting? When would you not use rate limiting?

Please reply and explain why you made this change.

	Andrew