diff mbox series

[net-next] net: dsa: mt7530: add support for changing DSA master

Message ID 20230210172942.13290-1-richard@routerhints.com (mailing list archive)
State New, archived
Headers show
Series [net-next] net: dsa: mt7530: add support for changing DSA master | expand

Commit Message

Arınç ÜNAL Feb. 10, 2023, 5:29 p.m. UTC 
From: Richard van Schagen <richard@routerhints.com>

Add support for changing the master of a port on the MT7530 DSA subdriver.

[ 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>
---
 drivers/net/dsa/mt7530.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Vladimir Oltean Feb. 10, 2023, 6:56 p.m. UTC | #1 
On Fri, Feb 10, 2023 at 08:29:43PM +0300, arinc9.unal@gmail.com wrote:
> From: Richard van Schagen <richard@routerhints.com>
> 
> Add support for changing the master of a port on the MT7530 DSA subdriver.
> 
> [ 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>
> ---
>  drivers/net/dsa/mt7530.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b5ad4b4fc00c..04bb4986454e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1072,6 +1072,38 @@ mt7530_port_disable(struct dsa_switch *ds, int port)
>  	mutex_unlock(&priv->reg_mutex);
>  }
>  
> +static int
> +mt7530_port_change_master(struct dsa_switch *ds, int port,
> +				       struct net_device *master,
> +				       struct netlink_ext_ack *extack)

alignment

> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +	int old_cpu = dp->cpu_dp->index;
> +	int new_cpu = cpu_dp->index;

I believe you need to reject LAG DSA masters.

> +
> +	mutex_lock(&priv->reg_mutex);
> +
> +	/* Move old to new cpu on User port */
> +	priv->ports[port].pm &= ~PCR_MATRIX(BIT(old_cpu));
> +	priv->ports[port].pm |= PCR_MATRIX(BIT(new_cpu));
> +
> +	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
> +		   priv->ports[port].pm);
> +
> +	/* Move user port from old cpu to new cpu */
> +	priv->ports[old_cpu].pm &= ~PCR_MATRIX(BIT(port));
> +	priv->ports[new_cpu].pm |= PCR_MATRIX(BIT(port));
> +
> +	mt7530_write(priv, MT7530_PCR_P(old_cpu), priv->ports[old_cpu].pm);
> +	mt7530_write(priv, MT7530_PCR_P(new_cpu), priv->ports[new_cpu].pm);

- who writes to the "pm" field of CPU ports?
- how does this line up with your other patch which said (AFAIU) that
  the port matrix of CPU ports should be 0 and that should be fine?
- read/modify/write (rmw) using PCR_MATRIX_MASK rather than mt7530_write().
  That overwrites the other PCR fields.

> +
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return 0;
> +}
> +
>  static int
>  mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
> @@ -3157,6 +3189,7 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.set_ageing_time	= mt7530_set_ageing_time,
>  	.port_enable		= mt7530_port_enable,
>  	.port_disable		= mt7530_port_disable,
> +	.port_change_master	= mt7530_port_change_master,
>  	.port_change_mtu	= mt7530_port_change_mtu,
>  	.port_max_mtu		= mt7530_port_max_mtu,
>  	.port_stp_state_set	= mt7530_stp_state_set,
> -- 
> 2.37.2
>
Richard van Schagen Feb. 10, 2023, 9:41 p.m. UTC | #2 
> On 10 Feb 2023, at 19:56, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> On Fri, Feb 10, 2023 at 08:29:43PM +0300, arinc9.unal@gmail.com wrote:
>> From: Richard van Schagen <richard@routerhints.com>
>> 
>> Add support for changing the master of a port on the MT7530 DSA subdriver.
>> 
>> [ 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>
>> ---
>> drivers/net/dsa/mt7530.c | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>> 
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index b5ad4b4fc00c..04bb4986454e 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -1072,6 +1072,38 @@ mt7530_port_disable(struct dsa_switch *ds, int port)
>> mutex_unlock(&priv->reg_mutex);
>> }
>> 
>> +static int
>> +mt7530_port_change_master(struct dsa_switch *ds, int port,
>> +        struct net_device *master,
>> +        struct netlink_ext_ack *extack)
> 
> alignment
> 
Will fix

>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + struct dsa_port *dp = dsa_to_port(ds, port);
>> + struct dsa_port *cpu_dp = master->dsa_ptr;
>> + int old_cpu = dp->cpu_dp->index;
>> + int new_cpu = cpu_dp->index;
> 
> I believe you need to reject LAG DSA masters.
> 

Not sure what you mean: how is this different from the change_master in the Felix driver when using 8021q tags?
But. Can add a check if you prefer. It might be a good idea anyway to be future proof. The MT7531 has support for LAG in hw.

>> +
>> + mutex_lock(&priv->reg_mutex);
>> +
>> + /* Move old to new cpu on User port */
>> + priv->ports[port].pm &= ~PCR_MATRIX(BIT(old_cpu));
>> + priv->ports[port].pm |= PCR_MATRIX(BIT(new_cpu));
>> +
>> + mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
>> +    priv->ports[port].pm);
>> +
>> + /* Move user port from old cpu to new cpu */
>> + priv->ports[old_cpu].pm &= ~PCR_MATRIX(BIT(port));
>> + priv->ports[new_cpu].pm |= PCR_MATRIX(BIT(port));
>> +
>> + mt7530_write(priv, MT7530_PCR_P(old_cpu), priv->ports[old_cpu].pm);
>> + mt7530_write(priv, MT7530_PCR_P(new_cpu), priv->ports[new_cpu].pm);
> 
> - who writes to the "pm" field of CPU ports?

Nobody actually writes to cpu.pm. I “fixed” that, and dropped that later. This is left over.

> - how does this line up with your other patch which said (AFAIU) that
>  the port matrix of CPU ports should be 0 and that should be fine?

Since cpu.pm was never user, and all user ports were added without using this field when enabling
the cpu, I changed that to add user ports belonging to that CPU. Arinc reported that it didn’t work.
Since the cpu.pm (empty) is writing during dsa_enable_port and that worked (for a long time already) the cpu.pm can be dropped.

> - read/modify/write (rmw) using PCR_MATRIX_MASK rather than mt7530_write().
>  That overwrites the other PCR fields.
> 

Good catch. Will fix.

>> +
>> + mutex_unlock(&priv->reg_mutex);
>> +
>> + return 0;
>> +}
>> +
>> static int
>> mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>> {
>> @@ -3157,6 +3189,7 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>> .set_ageing_time = mt7530_set_ageing_time,
>> .port_enable = mt7530_port_enable,
>> .port_disable = mt7530_port_disable,
>> + .port_change_master = mt7530_port_change_master,
>> .port_change_mtu = mt7530_port_change_mtu,
>> .port_max_mtu = mt7530_port_max_mtu,
>> .port_stp_state_set = mt7530_stp_state_set,
>> -- 
>> 2.37.2
Vladimir Oltean Feb. 10, 2023, 10:27 p.m. UTC | #3 
On Fri, Feb 10, 2023 at 09:41:06PM +0000, Richard van Schagen wrote:
> > I believe you need to reject LAG DSA masters.
> 
> Not sure what you mean: how is this different from the change_master in the Felix driver when using 8021q tags?
> But. Can add a check if you prefer. It might be a good idea anyway to be future proof. The MT7531 has support for LAG in hw.

I mean, like Documentation/networking/dsa/configuration.rst says, that the user can attempt
to put the DSA masters in a LAG and create a larger DSA master which is that bonding device.

The difference from the Felix driver is that Felix supports LAG DSA masters and this driver doesn't.

I don't believe there is any other restriction in the code which would prevent a driver which
implements port_change_master() from accepting that as a valid configuration, so it's going to
be the mt7530 driver who acts as the final frontier in this case.

An "if (netif_is_lag_master(master)) return -EOPNOTSUPP" will do. But it would always be good
to check if it's really needed :)
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b5ad4b4fc00c..04bb4986454e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1072,6 +1072,38 @@  mt7530_port_disable(struct dsa_switch *ds, int port)
 	mutex_unlock(&priv->reg_mutex);
 }
 
+static int
+mt7530_port_change_master(struct dsa_switch *ds, int port,
+				       struct net_device *master,
+				       struct netlink_ext_ack *extack)
+{
+	struct mt7530_priv *priv = ds->priv;
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+	int old_cpu = dp->cpu_dp->index;
+	int new_cpu = cpu_dp->index;
+
+	mutex_lock(&priv->reg_mutex);
+
+	/* Move old to new cpu on User port */
+	priv->ports[port].pm &= ~PCR_MATRIX(BIT(old_cpu));
+	priv->ports[port].pm |= PCR_MATRIX(BIT(new_cpu));
+
+	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
+		   priv->ports[port].pm);
+
+	/* Move user port from old cpu to new cpu */
+	priv->ports[old_cpu].pm &= ~PCR_MATRIX(BIT(port));
+	priv->ports[new_cpu].pm |= PCR_MATRIX(BIT(port));
+
+	mt7530_write(priv, MT7530_PCR_P(old_cpu), priv->ports[old_cpu].pm);
+	mt7530_write(priv, MT7530_PCR_P(new_cpu), priv->ports[new_cpu].pm);
+
+	mutex_unlock(&priv->reg_mutex);
+
+	return 0;
+}
+
 static int
 mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
@@ -3157,6 +3189,7 @@  static const struct dsa_switch_ops mt7530_switch_ops = {
 	.set_ageing_time	= mt7530_set_ageing_time,
 	.port_enable		= mt7530_port_enable,
 	.port_disable		= mt7530_port_disable,
+	.port_change_master	= mt7530_port_change_master,
 	.port_change_mtu	= mt7530_port_change_mtu,
 	.port_max_mtu		= mt7530_port_max_mtu,
 	.port_stp_state_set	= mt7530_stp_state_set,