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

[Andrew Lunn <andrew@lunn.ch>]

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