Re: Choose a default DSA CPU port

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, May 18, 2023 at 01:36:42PM +0300, Arınç ÜNAL wrote:
> The frames won't necessarily be trapped to the CPU port the user port is
> connected to. This operation is only there to make sure the trapped frames
> always reach the CPU.

That's kind of understated and I don't regard that as that big of a deal.
Since control packets cannot be guaranteed to be processed by the
conduit interface affine to the user port, I would go one step further
and say: don't even attempt to keep an affinity, just use for that purpose
the numerically first conduit interface that is up.

> I don't (know how to) check for other conduits being up when changing the
> trap port. So if a conduit is set down which results in both conduits being
> down, the trap port will still be changed to the other port which is
> unnecessary but it doesn't break anything.
> 
> Looking forward to your comments.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b5c8fdd381e5..55c11633f96f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -961,11 +961,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  	mt7530_set(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)) |
>  		   MT753X_UNM_FFP(BIT(port)) | MT753X_UNU_FFP(BIT(port)));
> -	/* Set CPU port number */
> -	if (priv->id == ID_MT7621)
> -		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
> -			   MT7530_CPU_PORT(port));
> -
>  	/* Add the CPU port to the CPU port bitmap for MT7531 and switch on
>  	 * MT7988 SoC. Any frames set for trapping to CPU port will be trapped
>  	 * to the CPU port the user port is connected to.
> @@ -2258,6 +2253,10 @@ mt7530_setup(struct dsa_switch *ds)
>  			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>  	}
> +	/* Trap BPDUs to the CPU port */
> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> +		   MT753X_BPDU_CPU_ONLY);
> +

This part will need its own patch + explanation
.
>  	/* Setup VLAN ID 0 for VLAN-unaware bridges */
>  	ret = mt7530_setup_vlan0(priv);
>  	if (ret)
> @@ -2886,6 +2885,50 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
>  	.pcs_an_restart = mt7530_pcs_an_restart,
>  };
> +static void
> +mt753x_master_state_change(struct dsa_switch *ds,
> +			   const struct net_device *master,
> +			   bool operational)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +	unsigned int trap_port;
> +
> +	/* Set the CPU port to trap frames to for MT7530. There can be only one
> +	 * CPU port due to MT7530_CPU_PORT having only 3 bits. Any frames set
> +	 * for trapping to CPU port will be trapped to the CPU port connected to
> +	 * the most recently set up DSA conduit. If the most recently set up DSA
> +	 * conduit is set down, frames will be trapped to the CPU port connected
> +	 * to the other DSA conduit.
> +	 */
> +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621) {

Just return early which saves one level of indentation.

	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
		return;

> +		trap_port = (mt7530_read(priv, MT753X_MFC) & MT7530_CPU_PORT_MASK) >> 4;
> +		dev_info(priv->dev, "trap_port is %d\n", trap_port);
> +		if (operational) {
> +			dev_info(priv->dev, "the conduit for cpu port %d is up\n", cpu_dp->index);
> +
> +			/* This check will be unnecessary if we find a way to
> +			 * not change the trap port to the other port when a
> +			 * conduit is set down which results in both conduits
> +			 * being down.
> +			 */
> +			if (!(cpu_dp->index == trap_port)) {
> +				dev_info(priv->dev, "trap to cpu port %d\n", cpu_dp->index);
> +				mt7530_set(priv, MT753X_MFC, MT7530_CPU_EN);
> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(cpu_dp->index));
> +			}
> +		} else {
> +			if (cpu_dp->index == 5 && trap_port == 5) {
> +				dev_info(priv->dev, "the conduit for cpu port 5 is down, trap frames to port 6\n");
> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(6));
> +			} else if (cpu_dp->index == 6 && trap_port == 6) {
> +				dev_info(priv->dev, "the conduit for cpu port 6 is down, trap frames to port 5\n");
> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(5));
> +			}
> +		}

I believe that the implementation where you cache the "operational"
value of previous calls will be cleaner. Something like this (written in
an email client, so take it with a grain of salt):

struct mt7530_priv {
	unsigned long active_cpu_ports;
	...
};

	if (operational)
		priv->active_cpu_ports |= BIT(cpu_dp->index);
	else
		priv->active_cpu_ports &= ~BIT(cpu_dp->index);

	if (priv->active_cpu_ports) {
		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
			   MT7530_CPU_EN |
			   MT7530_CPU_PORT(__ffs(priv->active_cpu_ports)));
	} else {
		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
			   MT7530_CPU_PORT(0));
	}

> +	}
> +}
> +
>  static int
>  mt753x_setup(struct dsa_switch *ds)
>  {
> @@ -2999,6 +3042,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
>  	.phylink_mac_link_up	= mt753x_phylink_mac_link_up,
>  	.get_mac_eee		= mt753x_get_mac_eee,
>  	.set_mac_eee		= mt753x_set_mac_eee,
> +	.master_state_change	= mt753x_master_state_change,
>  };
>  EXPORT_SYMBOL_GPL(mt7530_switch_ops);
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index fd2a2f726b8a..2abd3c5ce05a 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -41,8 +41,8 @@ enum mt753x_id {
>  #define  MT753X_UNU_FFP(x)		(((x) & 0xff) << 8)
>  #define  MT753X_UNU_FFP_MASK		MT753X_UNU_FFP(~0)
>  #define  MT7530_CPU_EN			BIT(7)
> -#define  MT7530_CPU_PORT(x)		((x) << 4)
> -#define  MT7530_CPU_MASK		(0xf << 4)
> +#define  MT7530_CPU_PORT(x)		(((x) & 0x7) << 4)
> +#define  MT7530_CPU_PORT_MASK		MT7530_CPU_PORT(~0)
>  #define  MT7530_MIRROR_EN		BIT(3)
>  #define  MT7530_MIRROR_PORT(x)		((x) & 0x7)
>  #define  MT7530_MIRROR_MASK		0x7
> 
> Arınç