| 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