Currently checksum is recalculated and dsa tag stripped even if we later
don't find the dev.
To improve code, exit early if we don't find the dev and skip additional
operation on the skb since it will be freed anyway.
Signed-off-by: Christian Marangi <[email protected]>
---
net/dsa/tag_qca.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index e757c8de06f1..e5ff7c34e577 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -75,10 +75,6 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
return NULL;
}
- /* Remove QCA tag and recalculate checksum */
- skb_pull_rcsum(skb, QCA_HDR_LEN);
- dsa_strip_etype_header(skb, QCA_HDR_LEN);
-
/* Get source port information */
port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, hdr);
@@ -86,6 +82,10 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
if (!skb->dev)
return NULL;
+ /* Remove QCA tag and recalculate checksum */
+ skb_pull_rcsum(skb, QCA_HDR_LEN);
+ dsa_strip_etype_header(skb, QCA_HDR_LEN);
+
return skb;
}
--
2.40.1
Address learning should initially be turned off by the driver for port
operation in standalone mode, then the DSA core handles changes to it
via ds->ops->port_bridge_flags().
Currently this is not the case for qca8k where learning is enabled
unconditionally in qca8k_setup for every user port.
Handle ports configured in standalone mode by making the learning
configurable and not enabling it by default.
Implement .port_pre_bridge_flags and .port_bridge_flags dsa ops to
enable learning for bridge that request it and tweak
.port_stp_state_set to correctly disable learning when port is
configured in standalone mode.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/dsa/qca/qca8k-8xxx.c | 7 +++--
drivers/net/dsa/qca/qca8k-common.c | 44 ++++++++++++++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 6 ++++
3 files changed, 54 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index ae088a4df794..31552853fdd4 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1870,9 +1870,8 @@ qca8k_setup(struct dsa_switch *ds)
if (ret)
return ret;
- /* Enable ARP Auto-learning by default */
- ret = regmap_set_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
- QCA8K_PORT_LOOKUP_LEARN);
+ ret = regmap_clear_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
+ QCA8K_PORT_LOOKUP_LEARN);
if (ret)
return ret;
@@ -1978,6 +1977,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
.port_change_mtu = qca8k_port_change_mtu,
.port_max_mtu = qca8k_port_max_mtu,
.port_stp_state_set = qca8k_port_stp_state_set,
+ .port_pre_bridge_flags = qca8k_port_pre_bridge_flags,
+ .port_bridge_flags = qca8k_port_bridge_flags,
.port_bridge_join = qca8k_port_bridge_join,
.port_bridge_leave = qca8k_port_bridge_leave,
.port_fast_age = qca8k_port_fast_age,
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index 13b8452ce5b2..e53694d2852a 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -565,9 +565,26 @@ int qca8k_get_mac_eee(struct dsa_switch *ds, int port,
return 0;
}
+static int qca8k_port_configure_learning(struct dsa_switch *ds, int port,
+ bool learning)
+{
+ struct qca8k_priv *priv = ds->priv;
+
+ if (learning)
+ return regmap_set_bits(priv->regmap,
+ QCA8K_PORT_LOOKUP_CTRL(port),
+ QCA8K_PORT_LOOKUP_LEARN);
+ else
+ return regmap_clear_bits(priv->regmap,
+ QCA8K_PORT_LOOKUP_CTRL(port),
+ QCA8K_PORT_LOOKUP_LEARN);
+}
+
void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
{
+ struct dsa_port *dp = dsa_to_port(ds, port);
struct qca8k_priv *priv = ds->priv;
+ bool learning = false;
u32 stp_state;
switch (state) {
@@ -582,8 +599,11 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
break;
case BR_STATE_LEARNING:
stp_state = QCA8K_PORT_LOOKUP_STATE_LEARNING;
+ learning = dp->learning;
break;
case BR_STATE_FORWARDING:
+ learning = dp->learning;
+ fallthrough;
default:
stp_state = QCA8K_PORT_LOOKUP_STATE_FORWARD;
break;
@@ -591,6 +611,30 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
+
+ qca8k_port_configure_learning(ds, port, learning);
+}
+
+int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack)
+{
+ if (flags.mask & ~BR_LEARNING)
+ return -EINVAL;
+
+ return 0;
+}
+
+int qca8k_port_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack)
+{
+ int ret;
+
+ ret = qca8k_port_configure_learning(ds, port,
+ flags.val & BR_LEARNING);
+
+ return ret;
}
int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index c5cc8a172d65..8f88b7db384d 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -522,6 +522,12 @@ int qca8k_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
/* Common bridge function */
void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
+int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack);
+int qca8k_port_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack);
int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
struct dsa_bridge bridge,
bool *tx_fwd_offload,
--
2.40.1
In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
the port loop and setup the LOOKUP MEMBER mask for user ports only to
the first CPU port.
This is to handle flooding condition where every CPU port is set as
target and prevent packet duplication for unknown frames from user ports.
Secondary CPU port LOOKUP MEMBER mask will be setup later when
port_change_master will be implemented.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/dsa/qca/qca8k-8xxx.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 31552853fdd4..6286a64a2fe3 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1850,18 +1850,16 @@ qca8k_setup(struct dsa_switch *ds)
if (ret)
return ret;
+ /* CPU port gets connected to all user ports of the switch */
+ ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(cpu_port),
+ QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
+ if (ret)
+ return ret;
+
/* Setup connection between CPU port & user ports
* Configure specific switch configuration for ports
*/
for (i = 0; i < QCA8K_NUM_PORTS; i++) {
- /* CPU port gets connected to all user ports of the switch */
- if (dsa_is_cpu_port(ds, i)) {
- ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
- QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
- if (ret)
- return ret;
- }
-
/* Individual user ports get connected to CPU port only */
if (dsa_is_user_port(ds, i)) {
ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
--
2.40.1
On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
>
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
>
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
>
> Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Mon, Jul 24, 2023 at 05:30:57AM +0200, Christian Marangi wrote:
> Address learning should initially be turned off by the driver for port
> operation in standalone mode, then the DSA core handles changes to it
> via ds->ops->port_bridge_flags().
>
> Currently this is not the case for qca8k where learning is enabled
> unconditionally in qca8k_setup for every user port.
>
> Handle ports configured in standalone mode by making the learning
> configurable and not enabling it by default.
>
> Implement .port_pre_bridge_flags and .port_bridge_flags dsa ops to
> enable learning for bridge that request it and tweak
> .port_stp_state_set to correctly disable learning when port is
> configured in standalone mode.
>
> Signed-off-by: Christian Marangi <[email protected]>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
...
> @@ -1978,6 +1977,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> .port_change_mtu = qca8k_port_change_mtu,
> .port_max_mtu = qca8k_port_max_mtu,
> .port_stp_state_set = qca8k_port_stp_state_set,
> + .port_pre_bridge_flags = qca8k_port_pre_bridge_flags,
> + .port_bridge_flags = qca8k_port_bridge_flags,
> .port_bridge_join = qca8k_port_bridge_join,
> .port_bridge_leave = qca8k_port_bridge_leave,
> .port_fast_age = qca8k_port_fast_age,
> diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
...
> @@ -591,6 +611,30 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>
> qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
> +
> + qca8k_port_configure_learning(ds, port, learning);
> +}
> +
> +int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> + struct switchdev_brport_flags flags,
> + struct netlink_ext_ack *extack)
> +{
> + if (flags.mask & ~BR_LEARNING)
> + return -EINVAL;
If I am reading things right then some implementation of this callback
return -EINVAL when they see unexpected flags. And some seem not to
- possibly because all flags are expected.
So I'm slightly unsure if this is correct or not.
> +
> + return 0;
> +}
...
On Mon, Jul 24, 2023 at 05:30:56AM +0200, Christian Marangi wrote:
> Currently checksum is recalculated and dsa tag stripped even if we later
> don't find the dev.
>
> To improve code, exit early if we don't find the dev and skip additional
> operation on the skb since it will be freed anyway.
>
> Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Mon, Jul 24, 2023 at 05:30:57AM +0200, Christian Marangi wrote:
> Address learning should initially be turned off by the driver for port
> operation in standalone mode, then the DSA core handles changes to it
> via ds->ops->port_bridge_flags().
>
> Currently this is not the case for qca8k where learning is enabled
> unconditionally in qca8k_setup for every user port.
>
> Handle ports configured in standalone mode by making the learning
> configurable and not enabling it by default.
>
> Implement .port_pre_bridge_flags and .port_bridge_flags dsa ops to
> enable learning for bridge that request it and tweak
> .port_stp_state_set to correctly disable learning when port is
> configured in standalone mode.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
Small nitpick below.
> drivers/net/dsa/qca/qca8k-8xxx.c | 7 +++--
> drivers/net/dsa/qca/qca8k-common.c | 44 ++++++++++++++++++++++++++++++
> drivers/net/dsa/qca/qca8k.h | 6 ++++
> 3 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index ae088a4df794..31552853fdd4 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1870,9 +1870,8 @@ qca8k_setup(struct dsa_switch *ds)
> if (ret)
> return ret;
>
> - /* Enable ARP Auto-learning by default */
> - ret = regmap_set_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
> - QCA8K_PORT_LOOKUP_LEARN);
> + ret = regmap_clear_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
> + QCA8K_PORT_LOOKUP_LEARN);
> if (ret)
> return ret;
>
> @@ -1978,6 +1977,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> .port_change_mtu = qca8k_port_change_mtu,
> .port_max_mtu = qca8k_port_max_mtu,
> .port_stp_state_set = qca8k_port_stp_state_set,
> + .port_pre_bridge_flags = qca8k_port_pre_bridge_flags,
> + .port_bridge_flags = qca8k_port_bridge_flags,
> .port_bridge_join = qca8k_port_bridge_join,
> .port_bridge_leave = qca8k_port_bridge_leave,
> .port_fast_age = qca8k_port_fast_age,
> diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
> index 13b8452ce5b2..e53694d2852a 100644
> --- a/drivers/net/dsa/qca/qca8k-common.c
> +++ b/drivers/net/dsa/qca/qca8k-common.c
> @@ -565,9 +565,26 @@ int qca8k_get_mac_eee(struct dsa_switch *ds, int port,
> return 0;
> }
>
> +static int qca8k_port_configure_learning(struct dsa_switch *ds, int port,
> + bool learning)
> +{
> + struct qca8k_priv *priv = ds->priv;
> +
> + if (learning)
> + return regmap_set_bits(priv->regmap,
> + QCA8K_PORT_LOOKUP_CTRL(port),
> + QCA8K_PORT_LOOKUP_LEARN);
> + else
> + return regmap_clear_bits(priv->regmap,
> + QCA8K_PORT_LOOKUP_CTRL(port),
> + QCA8K_PORT_LOOKUP_LEARN);
> +}
> +
> void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> {
> + struct dsa_port *dp = dsa_to_port(ds, port);
> struct qca8k_priv *priv = ds->priv;
> + bool learning = false;
> u32 stp_state;
>
> switch (state) {
> @@ -582,8 +599,11 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> break;
> case BR_STATE_LEARNING:
> stp_state = QCA8K_PORT_LOOKUP_STATE_LEARNING;
> + learning = dp->learning;
> break;
> case BR_STATE_FORWARDING:
> + learning = dp->learning;
> + fallthrough;
> default:
> stp_state = QCA8K_PORT_LOOKUP_STATE_FORWARD;
> break;
> @@ -591,6 +611,30 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>
> qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
> +
> + qca8k_port_configure_learning(ds, port, learning);
> +}
> +
> +int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> + struct switchdev_brport_flags flags,
> + struct netlink_ext_ack *extack)
> +{
> + if (flags.mask & ~BR_LEARNING)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +int qca8k_port_bridge_flags(struct dsa_switch *ds, int port,
> + struct switchdev_brport_flags flags,
> + struct netlink_ext_ack *extack)
> +{
> + int ret;
> +
> + ret = qca8k_port_configure_learning(ds, port,
> + flags.val & BR_LEARNING);
> +
> + return ret;
I worry that the way in this is formulated will attract patches from
kernel janitors to simplify it to:
return qca8k_port_configure_learning(...)
I agree that it's not strictly necessary to check flags.mask when
port_pre_bridge_flags supports a single flag, but if you did that and
structured the code for more future flags, you could avoid that.
int ret;
if (flags.mask & BR_LEARNING) {
ret = qca8k_port_configure_learning(...,
flags.val & BR_LEARNING);
if (ret)
return ret;
}
return 0;
Anyway, probably not a big deal.
> }
>
> int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index c5cc8a172d65..8f88b7db384d 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -522,6 +522,12 @@ int qca8k_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
>
> /* Common bridge function */
> void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
> +int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> + struct switchdev_brport_flags flags,
> + struct netlink_ext_ack *extack);
> +int qca8k_port_bridge_flags(struct dsa_switch *ds, int port,
> + struct switchdev_brport_flags flags,
> + struct netlink_ext_ack *extack);
> int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
> struct dsa_bridge bridge,
> bool *tx_fwd_offload,
> --
> 2.40.1
>
On Wed, Jul 26, 2023 at 10:19:34AM +0200, Simon Horman wrote:
> > +int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> > + struct switchdev_brport_flags flags,
> > + struct netlink_ext_ack *extack)
> > +{
> > + if (flags.mask & ~BR_LEARNING)
> > + return -EINVAL;
>
> If I am reading things right then some implementation of this callback
> return -EINVAL when they see unexpected flags. And some seem not to
> - possibly because all flags are expected.
>
> So I'm slightly unsure if this is correct or not.
Which ones don't? All handlers of SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS
should return -EINVAL for changes made to bridge port flags that aren't
supported.
On Mon, Jul 24, 2023 at 05:30:56AM +0200, Christian Marangi wrote:
> Currently checksum is recalculated and dsa tag stripped even if we later
> don't find the dev.
>
> To improve code, exit early if we don't find the dev and skip additional
> operation on the skb since it will be freed anyway.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
>
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
>
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
This is kinda "net.git" material, in the sense that it fixes the current
driver behavior with device trees from the future, right?
On Wed, Jul 26, 2023 at 03:14:35PM +0300, Vladimir Oltean wrote:
> On Wed, Jul 26, 2023 at 10:19:34AM +0200, Simon Horman wrote:
> > > +int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> > > + struct switchdev_brport_flags flags,
> > > + struct netlink_ext_ack *extack)
> > > +{
> > > + if (flags.mask & ~BR_LEARNING)
> > > + return -EINVAL;
> >
> > If I am reading things right then some implementation of this callback
> > return -EINVAL when they see unexpected flags. And some seem not to
> > - possibly because all flags are expected.
> >
> > So I'm slightly unsure if this is correct or not.
>
> Which ones don't? All handlers of SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS
> should return -EINVAL for changes made to bridge port flags that aren't
> supported.
Sorry, on a second look I see that my statement above is incorrect.
I do wonder what it was I saw this morning. But this afternoon
I see that all users check flags and return -EINVAL as appropriate.
On 7/23/23 20:30, Christian Marangi wrote:
> Currently checksum is recalculated and dsa tag stripped even if we later
> don't find the dev.
>
> To improve code, exit early if we don't find the dev and skip additional
> operation on the skb since it will be freed anyway.
>
> Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 7/23/23 20:30, Christian Marangi wrote:
> Address learning should initially be turned off by the driver for port
> operation in standalone mode, then the DSA core handles changes to it
> via ds->ops->port_bridge_flags().
>
> Currently this is not the case for qca8k where learning is enabled
> unconditionally in qca8k_setup for every user port.
>
> Handle ports configured in standalone mode by making the learning
> configurable and not enabling it by default.
>
> Implement .port_pre_bridge_flags and .port_bridge_flags dsa ops to
> enable learning for bridge that request it and tweak
> .port_stp_state_set to correctly disable learning when port is
> configured in standalone mode.
>
> Signed-off-by: Christian Marangi <[email protected]>
With the feedback from Vladimir being addressed:
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 7/23/23 20:30, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
>
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
>
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
>
> Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Wed, Jul 26, 2023 at 04:18:51PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> > In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> > the port loop and setup the LOOKUP MEMBER mask for user ports only to
> > the first CPU port.
> >
> > This is to handle flooding condition where every CPU port is set as
> > target and prevent packet duplication for unknown frames from user ports.
> >
> > Secondary CPU port LOOKUP MEMBER mask will be setup later when
> > port_change_master will be implemented.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
>
> This is kinda "net.git" material, in the sense that it fixes the current
> driver behavior with device trees from the future, right?
This is not strictly a fix. The secondary CPU (if defined) doesn't have
flood enabled so the switch won't forward packet. It's more of a
cleanup/preparation from my point of view. What do you think?
--
Ansuel
On Wed, Jul 26, 2023 at 03:12:13PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 24, 2023 at 05:30:57AM +0200, Christian Marangi wrote:
> > Address learning should initially be turned off by the driver for port
> > operation in standalone mode, then the DSA core handles changes to it
> > via ds->ops->port_bridge_flags().
> >
> > Currently this is not the case for qca8k where learning is enabled
> > unconditionally in qca8k_setup for every user port.
> >
> > Handle ports configured in standalone mode by making the learning
> > configurable and not enabling it by default.
> >
> > Implement .port_pre_bridge_flags and .port_bridge_flags dsa ops to
> > enable learning for bridge that request it and tweak
> > .port_stp_state_set to correctly disable learning when port is
> > configured in standalone mode.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
>
> Reviewed-by: Vladimir Oltean <[email protected]>
>
> Small nitpick below.
>
> > drivers/net/dsa/qca/qca8k-8xxx.c | 7 +++--
> > drivers/net/dsa/qca/qca8k-common.c | 44 ++++++++++++++++++++++++++++++
> > drivers/net/dsa/qca/qca8k.h | 6 ++++
> > 3 files changed, 54 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> > index ae088a4df794..31552853fdd4 100644
> > --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> > @@ -1870,9 +1870,8 @@ qca8k_setup(struct dsa_switch *ds)
> > if (ret)
> > return ret;
> >
> > - /* Enable ARP Auto-learning by default */
> > - ret = regmap_set_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
> > - QCA8K_PORT_LOOKUP_LEARN);
> > + ret = regmap_clear_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
> > + QCA8K_PORT_LOOKUP_LEARN);
> > if (ret)
> > return ret;
> >
> > @@ -1978,6 +1977,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> > .port_change_mtu = qca8k_port_change_mtu,
> > .port_max_mtu = qca8k_port_max_mtu,
> > .port_stp_state_set = qca8k_port_stp_state_set,
> > + .port_pre_bridge_flags = qca8k_port_pre_bridge_flags,
> > + .port_bridge_flags = qca8k_port_bridge_flags,
> > .port_bridge_join = qca8k_port_bridge_join,
> > .port_bridge_leave = qca8k_port_bridge_leave,
> > .port_fast_age = qca8k_port_fast_age,
> > diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
> > index 13b8452ce5b2..e53694d2852a 100644
> > --- a/drivers/net/dsa/qca/qca8k-common.c
> > +++ b/drivers/net/dsa/qca/qca8k-common.c
> > @@ -565,9 +565,26 @@ int qca8k_get_mac_eee(struct dsa_switch *ds, int port,
> > return 0;
> > }
> >
> > +static int qca8k_port_configure_learning(struct dsa_switch *ds, int port,
> > + bool learning)
> > +{
> > + struct qca8k_priv *priv = ds->priv;
> > +
> > + if (learning)
> > + return regmap_set_bits(priv->regmap,
> > + QCA8K_PORT_LOOKUP_CTRL(port),
> > + QCA8K_PORT_LOOKUP_LEARN);
> > + else
> > + return regmap_clear_bits(priv->regmap,
> > + QCA8K_PORT_LOOKUP_CTRL(port),
> > + QCA8K_PORT_LOOKUP_LEARN);
> > +}
> > +
> > void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> > {
> > + struct dsa_port *dp = dsa_to_port(ds, port);
> > struct qca8k_priv *priv = ds->priv;
> > + bool learning = false;
> > u32 stp_state;
> >
> > switch (state) {
> > @@ -582,8 +599,11 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> > break;
> > case BR_STATE_LEARNING:
> > stp_state = QCA8K_PORT_LOOKUP_STATE_LEARNING;
> > + learning = dp->learning;
> > break;
> > case BR_STATE_FORWARDING:
> > + learning = dp->learning;
> > + fallthrough;
> > default:
> > stp_state = QCA8K_PORT_LOOKUP_STATE_FORWARD;
> > break;
> > @@ -591,6 +611,30 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> >
> > qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> > QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
> > +
> > + qca8k_port_configure_learning(ds, port, learning);
> > +}
> > +
> > +int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> > + struct switchdev_brport_flags flags,
> > + struct netlink_ext_ack *extack)
> > +{
> > + if (flags.mask & ~BR_LEARNING)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +int qca8k_port_bridge_flags(struct dsa_switch *ds, int port,
> > + struct switchdev_brport_flags flags,
> > + struct netlink_ext_ack *extack)
> > +{
> > + int ret;
> > +
> > + ret = qca8k_port_configure_learning(ds, port,
> > + flags.val & BR_LEARNING);
> > +
> > + return ret;
>
> I worry that the way in this is formulated will attract patches from
> kernel janitors to simplify it to:
>
> return qca8k_port_configure_learning(...)
>
> I agree that it's not strictly necessary to check flags.mask when
> port_pre_bridge_flags supports a single flag, but if you did that and
> structured the code for more future flags, you could avoid that.
>
> int ret;
>
> if (flags.mask & BR_LEARNING) {
> ret = qca8k_port_configure_learning(...,
> flags.val & BR_LEARNING);
> if (ret)
> return ret;
> }
>
> return 0;
>
> Anyway, probably not a big deal.
>
> > }
I hope and expect to send fbd isolation later with FLOOD flags so I will
send v2 of this with the suggested format.
> >
> > int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
> > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > index c5cc8a172d65..8f88b7db384d 100644
> > --- a/drivers/net/dsa/qca/qca8k.h
> > +++ b/drivers/net/dsa/qca/qca8k.h
> > @@ -522,6 +522,12 @@ int qca8k_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
> >
> > /* Common bridge function */
> > void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
> > +int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> > + struct switchdev_brport_flags flags,
> > + struct netlink_ext_ack *extack);
> > +int qca8k_port_bridge_flags(struct dsa_switch *ds, int port,
> > + struct switchdev_brport_flags flags,
> > + struct netlink_ext_ack *extack);
> > int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
> > struct dsa_bridge bridge,
> > bool *tx_fwd_offload,
> > --
> > 2.40.1
> >
>
--
Ansuel
On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
>
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
>
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
> drivers/net/dsa/qca/qca8k-8xxx.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 31552853fdd4..6286a64a2fe3 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1850,18 +1850,16 @@ qca8k_setup(struct dsa_switch *ds)
> if (ret)
> return ret;
>
> + /* CPU port gets connected to all user ports of the switch */
> + ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(cpu_port),
> + QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
> + if (ret)
> + return ret;
> +
> /* Setup connection between CPU port & user ports
> * Configure specific switch configuration for ports
> */
> for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> - /* CPU port gets connected to all user ports of the switch */
> - if (dsa_is_cpu_port(ds, i)) {
> - ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
> - QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
> - if (ret)
> - return ret;
> - }
> -
> /* Individual user ports get connected to CPU port only */
> if (dsa_is_user_port(ds, i)) {
> ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
FWIW, the remaining loop can be rewritten (in a separate patch) using
dsa_switch_for_each_user_port(), which is actually an operation of lower
complexity compared to "for" + "dsa_is_user_port".
> --
> 2.40.1
>
On Thu, Jul 27, 2023 at 09:10:56PM +0200, Christian Marangi wrote:
> On Wed, Jul 26, 2023 at 04:18:51PM +0300, Vladimir Oltean wrote:
> > On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> > > In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> > > the port loop and setup the LOOKUP MEMBER mask for user ports only to
> > > the first CPU port.
> > >
> > > This is to handle flooding condition where every CPU port is set as
> > > target and prevent packet duplication for unknown frames from user ports.
> > >
> > > Secondary CPU port LOOKUP MEMBER mask will be setup later when
> > > port_change_master will be implemented.
> > >
> > > Signed-off-by: Christian Marangi <[email protected]>
> > > ---
> >
> > This is kinda "net.git" material, in the sense that it fixes the current
> > driver behavior with device trees from the future, right?
>
> This is not strictly a fix. The secondary CPU (if defined) doesn't have
> flood enabled so the switch won't forward packet. It's more of a
> cleanup/preparation from my point of view. What do you think?
>
> --
> Ansuel
Ah, ok, if packets don't reach the second CPU port anyway then it's fine.