[PATCH net-next v3 1/3] net: dsa: tag_yt921x: fix priority support

David Yang posted 3 patches 2 weeks ago
There is a newer version of this series
[PATCH net-next v3 1/3] net: dsa: tag_yt921x: fix priority support
Posted by David Yang 2 weeks ago
The packet priority is embedded in the rx tag. It defaults to 0, but
adding DCB support to the switch driver will break the tag driver by
setting it to non-zero.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 net/dsa/tag_yt921x.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_yt921x.c b/net/dsa/tag_yt921x.c
index 6bbfd42dc5df..b93715a057c7 100644
--- a/net/dsa/tag_yt921x.c
+++ b/net/dsa/tag_yt921x.c
@@ -17,7 +17,8 @@
  *   2: Rx Port
  *     15b: Rx Port Valid
  *     14b-11b: Rx Port
- *     10b-0b: Cmd?
+ *     10b-8b: Priority
+ *     7b-0b: Cmd
  *   2: Tx Port(s)
  *     15b: Tx Port(s) Valid
  *     10b-0b: Tx Port(s) Mask
@@ -33,7 +34,8 @@
 
 #define YT921X_TAG_PORT_EN		BIT(15)
 #define YT921X_TAG_RX_PORT_M		GENMASK(14, 11)
-#define YT921X_TAG_RX_CMD_M		GENMASK(10, 0)
+#define YT921X_TAG_RX_PRIO_M		GENMASK(10, 8)
+#define YT921X_TAG_RX_CMD_M		GENMASK(7, 0)
 #define  YT921X_TAG_RX_CMD(x)			FIELD_PREP(YT921X_TAG_RX_CMD_M, (x))
 #define  YT921X_TAG_RX_CMD_FORWARDED		0x80
 #define  YT921X_TAG_RX_CMD_UNK_UCAST		0xb2
@@ -98,6 +100,8 @@ yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
 		return NULL;
 	}
 
+	skb->priority = FIELD_GET(YT921X_TAG_RX_PRIO_M, rx);
+
 	cmd = FIELD_GET(YT921X_TAG_RX_CMD_M, rx);
 	switch (cmd) {
 	case YT921X_TAG_RX_CMD_FORWARDED:
-- 
2.51.0
Re: [net-next,v3,1/3] net: dsa: tag_yt921x: fix priority support
Posted by Simon Horman 1 week, 5 days ago
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: yt921x: Add DCB/QoS support

This commit adds DCB/QoS support to the yt921x DSA switch driver,
setting up global DSCP/PCP priority mappings and adding related DCB
methods.

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index af8464c42b51..87e33d0df4ac 100644
> --- a/drivers/net/dsa/yt921x.c
> +++ b/drivers/net/dsa/yt921x.c

[ ... ]

> +/* Not "port" - DSCP mapping is global */
> +static int __maybe_unused
> +yt921x_dsa_port_get_dscp_prio(struct dsa_switch *ds, int port, u8 dscp)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	u32 val;
> +	int res;
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt921x_reg_read(priv, YT921X_IPM_DSCPn(dscp), &val);
> +	mutex_unlock(&priv->reg_lock);
> +
> +	if (res)
> +		return res;
> +
> +	return FIELD_GET(YT921X_IPM_PRIO_M, val);
> +}
> +
> +static int __maybe_unused
> +yt921x_dsa_port_del_dscp_prio(struct dsa_switch *ds, int port, u8 dscp, u8 prio)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	int res;
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt921x_reg_write(priv, YT921X_IPM_DSCPn(dscp),
> +			       YT921X_IPM_PRIO(IEEE8021Q_TT_BK));
> +	mutex_unlock(&priv->reg_lock);
> +
> +	return res;
> +}

Should yt921x_dsa_port_del_dscp_prio() check the prio parameter before
resetting the DSCP mapping? The ocelot driver (ocelot_port_del_dscp_prio
in drivers/net/ethernet/mscc/ocelot.c) has a comment explaining that
during a "dcb app replace" command, the new entry is added first, then
the old entry is deleted. If the delete operation ignores the prio
parameter, it will incorrectly reset the newly-added mapping instead
of only deleting when the current priority matches the old one.

The ksz driver also checks this condition in ksz_port_del_dscp_prio().

Without this check, the sequence for "dcb app replace ... dscp X prio N"
followed by "dcb app replace ... dscp X prio M" would be:
  1. add dscp X -> prio M (correct)
  2. del dscp X, prio N (but code ignores prio N and resets to BK)
  3. result: dscp X -> BK instead of prio M

Perhaps something like this is needed:

  if (yt921x_dsa_port_get_dscp_prio(ds, port, dscp) != prio)
      return 0;