On Tue, Mar 08, 2022 at 19:17, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> On Tue, Mar 08, 2022 at 09:01:04AM +0100, Tobias Waldekranz wrote:
>> On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
>> > On Tue, Mar 01, 2022 at 11:03:15AM +0100, Tobias Waldekranz wrote:
>> >> Whenever a VLAN moves to a new MSTI, send a switchdev notification so
>> >> that switchdevs can...
>> >>
>> >> ...either refuse the migration if the hardware does not support
>> >> offloading of MST...
>> >>
>> >> ..or track a bridge's VID to MSTI mapping when offloading is
>> >> supported.
>> >>
>> >> Signed-off-by: Tobias Waldekranz <tobias@xxxxxxxxxxxxxx>
>> >> ---
>> >> include/net/switchdev.h | 10 +++++++
>> >> net/bridge/br_mst.c | 15 +++++++++++
>> >> net/bridge/br_switchdev.c | 57 +++++++++++++++++++++++++++++++++++++++
>> >> 3 files changed, 82 insertions(+)
>> >>
>> >> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> >> index 3e424d40fae3..39e57aa5005a 100644
>> >> --- a/include/net/switchdev.h
>> >> +++ b/include/net/switchdev.h
>> >> @@ -28,6 +28,7 @@ enum switchdev_attr_id {
>> >> SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
>> >> SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
>> >> SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
>> >> + SWITCHDEV_ATTR_ID_VLAN_MSTI,
>> >> };
>> >>
>> >> struct switchdev_brport_flags {
>> >> @@ -35,6 +36,14 @@ struct switchdev_brport_flags {
>> >> unsigned long mask;
>> >> };
>> >>
>> >> +struct switchdev_vlan_attr {
>> >> + u16 vid;
>> >> +
>> >> + union {
>> >> + u16 msti;
>> >> + };
>> >
>> > Do you see other VLAN attributes that would be added in the future, such
>> > as to justify making this a single-element union from the get-go?
>>
>> I could imagine being able to control things like multicast snooping on
>> a per-VLAN basis. Being able to act as a multicast router in one VLAN
>> but not another.
>>
>> > Anyway if that is the case, we're lacking an id for the attribute type,
>> > so we'd end up needing to change drivers when a second union element
>> > appears. Otherwise they'd all expect an u16 msti.
>>
>> My idea was that `enum switchdev_attr_id` would hold all of that
>> information. In this example SWITCHDEV_ATTR_ID_VLAN_MSTI, denotes both
>> that `vlan_attr` is the valid member of `u` and that `msti` is the valid
>> member of `vlan_attr`. If we add SWITCHDEV_ATTR_ID_VLAN_SNOOPING, that
>> would point to both `vlan_attr` and a new `bool snooping` in the union.
>>
>> Do you think we should just have a SWITCHDEV_ATTR_ID_VLAN_ATTR for all
>> per-VLAN attributes and then have a separate union?
>
> It's the first nested union that I see, and a bit confusing.
>
> I think it would be better if we had a
>
> struct switchdev_vlan_attr_msti {
> u16 vid;
> u16 msti;
> };
>
> and different structures for other, future VLAN attributes. Basically
> keep a 1:1 mapping between an attribute id and a union.
Yeah, I like the simplicity of that. Changing.
>> >> +};
>> >> +
>> >> struct switchdev_attr {
>> >> struct net_device *orig_dev;
>> >> enum switchdev_attr_id id;
>> >> @@ -50,6 +59,7 @@ struct switchdev_attr {
>> >> u16 vlan_protocol; /* BRIDGE_VLAN_PROTOCOL */
>> >> bool mc_disabled; /* MC_DISABLED */
>> >> u8 mrp_port_role; /* MRP_PORT_ROLE */
>> >> + struct switchdev_vlan_attr vlan_attr; /* VLAN_* */
>> >> } u;
>> >> };
>> >>
>> >> diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
>> >> index 8dea8e7257fd..aba603675165 100644
>> >> --- a/net/bridge/br_mst.c
>> >> +++ b/net/bridge/br_mst.c
>> >> @@ -7,6 +7,7 @@
>> >> */
>> >>
>> >> #include <linux/kernel.h>
>> >> +#include <net/switchdev.h>
>> >>
>> >> #include "br_private.h"
>> >>
>> >> @@ -65,9 +66,23 @@ static void br_mst_vlan_sync_state(struct net_bridge_vlan *pv, u16 msti)
>> >>
>> >> int br_mst_vlan_set_msti(struct net_bridge_vlan *mv, u16 msti)
>> >> {
>> >> + struct switchdev_attr attr = {
>> >> + .id = SWITCHDEV_ATTR_ID_VLAN_MSTI,
>> >> + .flags = SWITCHDEV_F_DEFER,
>> >
>> > Is the bridge spinlock held (atomic context), or otherwise why is
>> > SWITCHDEV_F_DEFER needed here?
>>
>> Nope, just copypasta. In fact, it shouldn't be needed when setting the
>> state either, as you can only change the state via a netlink message. I
>> will remove it.
>>
>> >> + .orig_dev = mv->br->dev,
>> >> + .u.vlan_attr = {
>> >> + .vid = mv->vid,
>> >> + .msti = msti,
>> >> + },
>> >> + };
>> >> struct net_bridge_vlan_group *vg;
>> >> struct net_bridge_vlan *pv;
>> >> struct net_bridge_port *p;
>> >> + int err;
>> >> +
>> >> + err = switchdev_port_attr_set(mv->br->dev, &attr, NULL);
>> >
>> > Treating a "VLAN attribute" as a "port attribute of the bridge" is
>> > pushing the taxonomy just a little, but I don't have a better suggestion.
>>
>> Isn't there prior art here? I thought things like VLAN filtering already
>> worked like this?
>
> Hmm, I can think of VLAN filtering as being an attribute of the bridge
> device, but 'which MSTI does VLAN X belong to' is an attribute of the
> VLAN (in itself a switchdev object, i.e. something countable).
>
> If the prior art would apply as straightforward as you say, then we'd be
> replaying the VLAN MSTIs together with the other port attributes - in
> "pull" mode, in dsa_port_switchdev_sync_attrs(), rather than in "push"
> mode with the rest of the objects - in nbp_switchdev_sync_objs().
> But we're not doing that.
>
> To prove that there is a difference between VLAN filtering as a port
> property of the bridge device, and VLAN MSTIs (or other per-VLAN global
> bridge options), consider this.
> You create a bridge, add 10 VLANs on br0, enable VLAN filtering, then
> delete the 10 VLANs and re-create them. The bridge is still VLAN
> filtering.
> So VLAN filtering is a property of the bridge.
>
> Next you create a bridge, add 10 VLANs on br0, run your new command:
> 'bridge vlan global set dev br0 vid <VID> msti <MSTI>'
> then delete the 10 VLANs and create them back.
> Their MSTI is 0, not what was set via the bridge vlan global options...
> Because the MSTI is a property of the VLANs, not of the bridge.
>
> A real port attribute wouldn't behave like that.
>
> At least this is what I understand from your patch set, I haven't run it;
> sorry if I'm mistaken about something, but I can't find a clearer way to
> express what I find strange.
>
> Anyway, I'll stop uselessly commenting here - I can understand the
> practical reasons why you wouldn't want to bother expanding the taxonomy
> to describe this for what it really is - an "object attribute" of sorts -
> because a port attribute for the bridge device has the call path you
> need already laid out, including replication towards all bridge ports.
I yield, I yield! :)
