| 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: <e140cec6-132c-0e3a-d48a-88cd176b9875@arinc9.com>
Date: Fri, 19 May 2023 12:00:17 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Vladimir Oltean <olteanv@...il.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 18.05.2023 17:24, Vladimir Oltean wrote:
> 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.
Makes sense. Good thing the MT7531 switch is capable of having the control
packets processed by the DSA conduit interface affine to the user port so
it's only the MT7530 switch that we need to implement "don't even attempt
to keep an affinity, just use the numerically first conduit interface that
is up" for.
>
>> 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
Will split.
> .
>> /* 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;
Will do.
>
>> + 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)));
This is nice and simple, thank you.
> } else {
> mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
> MT7530_CPU_PORT(0));
If I understand correctly, the MT7530_CPU_EN bit here wouldn't be modified
since it's not on the set parameter. On top of this, I believe we can
completely get rid of the else case. The MT7530_CPU_PORT bits will be
overwritten when there's an active CPU port so there's no need to clear
them when there's no active CPU ports. MT7530_CPU_EN might as well stay
set.
Arınç
Powered by blists - more mailing lists