lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230518142422.62hm5d4orvy7nroz@skbuf> Date: Thu, 18 May 2023 17:24:22 +0300 From: Vladimir Oltean <olteanv@...il.com> To: Arınç ÜNAL <arinc.unal@...nc9.com> Cc: Frank Wunderlich <frank-w@...lic-files.de>, Felix Fietkau <nbd@....name>, netdev <netdev@...r.kernel.org>, erkin.bozoglu@...ont.com, Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, John Crispin <john@...ozen.org>, Mark Lee <Mark-MC.Lee@...iatek.com>, Lorenzo Bianconi <lorenzo@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>, Landen Chao <Landen.Chao@...iatek.com>, Sean Wang <sean.wang@...iatek.com>, DENG Qingfang <dqfext@...il.com>, mithat.guner@...ont.com Subject: Re: Choose a default DSA CPU port 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ç
Powered by blists - more mailing lists