On Tue, Mar 15, 2022 at 01:25:38AM +0100, Tobias Waldekranz wrote:
> When joining a bridge where MST is enabled, we validate that the
> proper offloading support is in place, otherwise we fallback to
> software bridging.
>
> When then mode is changed on a bridge in which we are members, we
> refuse the change if offloading is not supported.
>
> At the moment we only check for configurable learning, but this will
> be further restricted as we support more MST related switchdev events.
>
> Signed-off-by: Tobias Waldekranz <[email protected]>
> ---
> net/dsa/dsa_priv.h | 2 ++
> net/dsa/port.c | 22 ++++++++++++++++++++++
> net/dsa/slave.c | 6 ++++++
> 3 files changed, 30 insertions(+)
>
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index f20bdd8ea0a8..2aba420696ef 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -234,6 +234,8 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
> struct netlink_ext_ack *extack);
> bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
> int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock);
> +int dsa_port_mst_enable(struct dsa_port *dp, bool on,
> + struct netlink_ext_ack *extack);
> int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu,
> bool targeted_match);
> int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 58291df14cdb..02214033cec0 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -321,6 +321,11 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp,
> kfree(bridge);
> }
>
> +static bool dsa_port_supports_mst(struct dsa_port *dp)
> +{
> + return dsa_port_can_configure_learning(dp);
> +}
> +
> int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
> struct netlink_ext_ack *extack)
> {
> @@ -334,6 +339,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
> struct net_device *brport_dev;
> int err;
>
> + if (br_mst_enabled(br) && !dsa_port_supports_mst(dp))
> + return -EOPNOTSUPP;
> +
> /* Here the interface is already bridged. Reflect the current
> * configuration so that drivers can program their chips accordingly.
> */
> @@ -735,6 +743,20 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock)
> return 0;
> }
>
> +int dsa_port_mst_enable(struct dsa_port *dp, bool on,
> + struct netlink_ext_ack *extack)
> +{
> + if (!on)
> + return 0;
> +
> + if (!dsa_port_supports_mst(dp)) {
> + NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Experimenting a bit... maybe this looks tidier? We make the "if" condition
have the same basic structure as the previous "if (br_mst_enabled(br) &&
!dsa_port_supports_mst(dp))", albeit transformed using De Morgan's rules.
{
if (!on || dsa_port_supports_mst(dp))
return 0;
NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
return -EINVAL;
}
> +
> int dsa_port_pre_bridge_flags(const struct dsa_port *dp,
> struct switchdev_brport_flags flags,
> struct netlink_ext_ack *extack)
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 647adee97f7f..879d18cc99cb 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -463,6 +463,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx,
>
> ret = dsa_port_ageing_time(dp, attr->u.ageing_time);
> break;
> + case SWITCHDEV_ATTR_ID_BRIDGE_MST:
> + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev))
> + return -EOPNOTSUPP;
> +
> + ret = dsa_port_mst_enable(dp, attr->u.mst, extack);
> + break;
> case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
> if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev))
> return -EOPNOTSUPP;
> --
> 2.25.1
>
On Tue, Mar 15, 2022 at 19:11, Vladimir Oltean <[email protected]> wrote:
> On Tue, Mar 15, 2022 at 01:25:38AM +0100, Tobias Waldekranz wrote:
>> When joining a bridge where MST is enabled, we validate that the
>> proper offloading support is in place, otherwise we fallback to
>> software bridging.
>>
>> When then mode is changed on a bridge in which we are members, we
>> refuse the change if offloading is not supported.
>>
>> At the moment we only check for configurable learning, but this will
>> be further restricted as we support more MST related switchdev events.
>>
>> Signed-off-by: Tobias Waldekranz <[email protected]>
>> ---
>> net/dsa/dsa_priv.h | 2 ++
>> net/dsa/port.c | 22 ++++++++++++++++++++++
>> net/dsa/slave.c | 6 ++++++
>> 3 files changed, 30 insertions(+)
>>
>> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
>> index f20bdd8ea0a8..2aba420696ef 100644
>> --- a/net/dsa/dsa_priv.h
>> +++ b/net/dsa/dsa_priv.h
>> @@ -234,6 +234,8 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
>> struct netlink_ext_ack *extack);
>> bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
>> int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock);
>> +int dsa_port_mst_enable(struct dsa_port *dp, bool on,
>> + struct netlink_ext_ack *extack);
>> int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu,
>> bool targeted_match);
>> int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
>> diff --git a/net/dsa/port.c b/net/dsa/port.c
>> index 58291df14cdb..02214033cec0 100644
>> --- a/net/dsa/port.c
>> +++ b/net/dsa/port.c
>> @@ -321,6 +321,11 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp,
>> kfree(bridge);
>> }
>>
>> +static bool dsa_port_supports_mst(struct dsa_port *dp)
>> +{
>> + return dsa_port_can_configure_learning(dp);
>> +}
>> +
>> int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>> struct netlink_ext_ack *extack)
>> {
>> @@ -334,6 +339,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>> struct net_device *brport_dev;
>> int err;
>>
>> + if (br_mst_enabled(br) && !dsa_port_supports_mst(dp))
>> + return -EOPNOTSUPP;
>> +
>> /* Here the interface is already bridged. Reflect the current
>> * configuration so that drivers can program their chips accordingly.
>> */
>> @@ -735,6 +743,20 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock)
>> return 0;
>> }
>>
>> +int dsa_port_mst_enable(struct dsa_port *dp, bool on,
>> + struct netlink_ext_ack *extack)
>> +{
>> + if (!on)
>> + return 0;
>> +
>> + if (!dsa_port_supports_mst(dp)) {
>> + NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>
> Experimenting a bit... maybe this looks tidier? We make the "if" condition
> have the same basic structure as the previous "if (br_mst_enabled(br) &&
> !dsa_port_supports_mst(dp))", albeit transformed using De Morgan's rules.
>
> {
> if (!on || dsa_port_supports_mst(dp))
> return 0;
>
> NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
> return -EINVAL;
> }
I initially had it like this. It looks tidier, yes - but to me the
intent is less obvious when reading it. How about:
{
if (on && !dsa_port_supports_mst(dp)) {
NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
return -EINVAL;
}
return 0;
}
>> +
>> int dsa_port_pre_bridge_flags(const struct dsa_port *dp,
>> struct switchdev_brport_flags flags,
>> struct netlink_ext_ack *extack)
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 647adee97f7f..879d18cc99cb 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -463,6 +463,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx,
>>
>> ret = dsa_port_ageing_time(dp, attr->u.ageing_time);
>> break;
>> + case SWITCHDEV_ATTR_ID_BRIDGE_MST:
>> + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev))
>> + return -EOPNOTSUPP;
>> +
>> + ret = dsa_port_mst_enable(dp, attr->u.mst, extack);
>> + break;
>> case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
>> if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev))
>> return -EOPNOTSUPP;
>> --
>> 2.25.1
>>