1. Patchew
  2. linux
  3. [PATCH net v5 0/6] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling
  4. View patch

[PATCH net v5 2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

arinc9.unal@gmail.com posted 6 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH net v5 2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
Posted by arinc9.unal@gmail.com 1 year, 3 months ago 
From: Arınç ÜNAL <arinc.unal@arinc9.com>

The check for setting the CPU_PORT bits must include the non-MT7621 SoC
MT7530 switch variants to trap frames. Expand the check to include them.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0a5237793209..e9fbe7ae6c2c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1007,7 +1007,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
 		   UNU_FFP(BIT(port)));
 
 	/* Set CPU port number */
-	if (priv->id == ID_MT7621)
+	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
 		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
 
 	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
-- 
2.39.2
Re: [PATCH net v5 2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
Posted by Vladimir Oltean 1 year, 3 months ago 
On Fri, Jun 16, 2023 at 05:53:23AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames. Expand the check to include them.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Maybe this alternative commit message is an improvement:

All MT7530 switch IP variants share the MT7530_MFC register, but the
current driver only writes it for the switch variant that is integrated
in the MT7621 SoC. Modify the code to include all MT7530 derivatives.
Re: [PATCH net v5 2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
Posted by Vladimir Oltean 1 year, 3 months ago 
On Fri, Jun 16, 2023 at 05:53:23AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames.

I would add a simple "(identified by ID_MT7530)" to the commit message here.

> Expand the check to include them.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/mt7530.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0a5237793209..e9fbe7ae6c2c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1007,7 +1007,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  		   UNU_FFP(BIT(port)));
>  
>  	/* Set CPU port number */
> -	if (priv->id == ID_MT7621)
> +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
>  		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
>  
>  	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
> -- 
> 2.39.2
>
Re: [PATCH net v5 2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
Posted by Vladimir Oltean 1 year, 3 months ago 
On Fri, Jun 16, 2023 at 05:53:23AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames. Expand the check to include them.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

why do you say non-MT7621 when the change specifically includes MT7621?
What is the affected SoC then?

>  drivers/net/dsa/mt7530.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0a5237793209..e9fbe7ae6c2c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1007,7 +1007,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  		   UNU_FFP(BIT(port)));
>  
>  	/* Set CPU port number */
> -	if (priv->id == ID_MT7621)
> +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
>  		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
>  
>  	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
> -- 
> 2.39.2
>
Re: [PATCH net v5 2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
Posted by Russell King (Oracle) 1 year, 3 months ago 
On Fri, Jun 16, 2023 at 01:03:14PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 05:53:23AM +0300, arinc9.unal@gmail.com wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> > MT7530 switch variants to trap frames. Expand the check to include them.
> > 
> > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > ---
> 
> why do you say non-MT7621 when the change specifically includes MT7621?
> What is the affected SoC then?

Thanks for falling into one of the issues that makes reviewing these
patches difficult. :/

> > -	if (priv->id == ID_MT7621)
> > +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
> >  		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));

I *think* what the commit message should be saying is that the setup
for the CPU port(*) is necessary not only for MT7621, but also for
MT7530 variants as well.

That can be construed from the commit message, but it doesn't easily
read that way.

* - in this case, it's the CPU port field and the CPU enable bit.
Note that CPU_MASK only covers CPU_PORT() and not CPU_EN, but this
doesn't matter for mt7530_rmw().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH net v5 2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
Posted by Vladimir Oltean 1 year, 3 months ago 
On Fri, Jun 16, 2023 at 11:17:54AM +0100, Russell King (Oracle) wrote:
> On Fri, Jun 16, 2023 at 01:03:14PM +0300, Vladimir Oltean wrote:
> > On Fri, Jun 16, 2023 at 05:53:23AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> > > MT7530 switch variants to trap frames. Expand the check to include them.
> > > 
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > 
> > why do you say non-MT7621 when the change specifically includes MT7621?
> > What is the affected SoC then?
> 
> Thanks for falling into one of the issues that makes reviewing these
> patches difficult. :/
> 
> > > -	if (priv->id == ID_MT7621)
> > > +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
> > >  		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
> 
> I *think* what the commit message should be saying is that the setup
> for the CPU port(*) is necessary not only for MT7621, but also for
> MT7530 variants as well.
> 
> That can be construed from the commit message, but it doesn't easily
> read that way.
> 
> * - in this case, it's the CPU port field and the CPU enable bit.
> Note that CPU_MASK only covers CPU_PORT() and not CPU_EN, but this
> doesn't matter for mt7530_rmw().
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Ah, no, I just misread the patch, because the new term was added to the
left of the existing one, and not to the right as I would have expected.
I though it was this:

> > > -	if (priv->id == ID_MT7530)
> > > +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)

thus my confusion and my question.

I'm okay with the change now.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.