Date:   Fri, 10 Feb 2023 20:44:09 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     arinc9.unal@gmail.com
Cc:     Sean Wang <sean.wang@mediatek.com>,
        Landen Chao <Landen.Chao@mediatek.com>,
        DENG Qingfang <dqfext@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Richard van Schagen <richard@routerhints.com>,
        =?utf-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, erkin.bozoglu@xeront.com
Subject: Re: [PATCH net] net: dsa: mt7530: fix CPU flooding and do not set
 CPU association
Message-ID: <20230210184409.e6ueolfdsmhqfph5@skbuf>
References: <20230210172822.12960-1-richard@routerhints.com>
 <20230210172822.12960-1-richard@routerhints.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230210172822.12960-1-richard@routerhints.com>
 <20230210172822.12960-1-richard@routerhints.com>
Precedence: bulk

Hi,

On Fri, Feb 10, 2023 at 08:28:23PM +0300, arinc9.unal@gmail.com wrote:
> From: Richard van Schagen <richard@routerhints.com>
> 
> The original code only enables flooding on CPU port, on port 6, since
> that's the last one set up. In doing so, it removes flooding on port 5,
> which made so that, in order to communicate properly over port 5, a frame
> had to be sent from a user port to the DSA master. Fix this.

Separate patch for this. I don't understand the correlation with the
other part below.

FWIW, the problem can also be solved similar to 8d5f7954b7c8 ("net: dsa:
felix: break at first CPU port during init and teardown"), and both CPU
ports could be added to the flooding mask only as part of the "multiple
CPU ports" feature. When a multiple CPU ports device tree is used with a
kernel capable of a single CPU port, your patch enables flooding towards
the second CPU port, which will never be used (or up). Not sure if you
want that.

> 
> Since CPU->port is forced via the DSA tag, connecting CPU to all user ports
> of the switch breaks communication over VLAN tagged frames.

Here, I understand almost nothing from this phrase.

"CPU->port" means "association between user port and CPU port"?

You're saying that association is forced through the DSA tag? Details?
Who or what is the DSA tag? Or are you saying that packets transmitted
by tag_mtk.c are always sent as control plane, and will reach an egress
port regardless of the port matrix of the CPU port?

"Connecting CPU to all user ports" means assigning PCR_MATRIX(dsa_user_ports())
to the port matrix of the CPU port, yes? Why would that break communication
for VLAN-tagged traffic (and what is the source and destination of that
traffic)?

> Therefore, remove the code that sets CPU assocation.
> This way, the CPU reverts to not being connected to any port as soon
> as ".port_enable" is called.

Partly to blame may be the poor phrasing here. AFAICS, the port matrix
of the CPU port remains 0 throughout the lifetime of the driver. Why
mention ".port_enable"? That handles the user -> CPU direction, not the
CPU -> user direction.

> 
> [ arinc.unal@arinc9.com: Wrote subject and changelog ]
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Richard van Schagen <richard@routerhints.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Missing Fixes: tags for patches sent to "net". Multiple problems =>
multiple patches.

> ---
>  drivers/net/dsa/mt7530.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3a15015bc409..b5ad4b4fc00c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -997,6 +997,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  	int ret;
> +	u32 val;
>  
>  	/* Setup max capability of CPU port at first */
>  	if (priv->info->cpu_port_config) {
> @@ -1009,20 +1010,15 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  	mt7530_write(priv, MT7530_PVC_P(port),
>  		     PORT_SPEC_TAG);
>  
> -	/* Disable flooding by default */
> -	mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK,
> -		   BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
> +	/* Enable flooding on CPU */
> +	val = mt7530_read(priv, MT7530_MFC);
> +	val |= BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port));
> +	mt7530_write(priv, MT7530_MFC, val);
>  
>  	/* Set CPU port number */
>  	if (priv->id == ID_MT7621)
>  		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
>  
> -	/* CPU port gets connected to all user ports of
> -	 * the switch.
> -	 */
> -	mt7530_write(priv, MT7530_PCR_P(port),
> -		     PCR_MATRIX(dsa_user_ports(priv->ds)));
> -
>  	/* Set to fallback mode for independent VLAN learning */
>  	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
>  		   MT7530_PORT_FALLBACK_MODE);
> @@ -2204,6 +2200,9 @@ mt7530_setup(struct dsa_switch *ds)
>  
>  	priv->p6_interface = PHY_INTERFACE_MODE_NA;
>  
> +	/* Disable flooding by default */
> +	mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK, 0);
> +

Shouldn't mt7531_setup() have this too?

>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
>  
> -- 
> 2.37.2
>