Hi Vivien,

On Mon, 26 Aug 2019 at 18:20, Vivien Didelot [off-list ref] wrote:

Hi Vladimir,

On Sun, 25 Aug 2019 21:44:54 +0300, Vladimir Oltean [off-list ref] wrote:

quoted

-     if (enabled)
-             err = dsa_port_vid_add(upstream_dp, tx_vid, 0);
-     else
-             err = dsa_port_vid_del(upstream_dp, tx_vid);
+     err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
      if (err) {
              dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
                      tx_vid, upstream, err);
              return err;
      }

-     return 0;
+     if (!enabled)
+             err = dsa_8021q_restore_pvid(ds, port);
+
+     return err;
 }

I did not dig that much into tag_8021q.c yet. From seeing this portion,
I'm just wondering if these two helpers couldn't be part of the same logic
as they both act upon the "enabled" condition?

Otherwise I have no complains about the series.

I thought too about trying to merge the 2 into the same function (not
a lot, though).
But consider that they do different things in the "!enabled" case:
- dsa_8021q_vid_apply: check if this specific vid (provided as
argument) was installed in the bridge, and if so, restore it
- dsa_8021q_restore_pvid: search for the bridge port's pvid, and restore that
I don't think that the end result will look cleaner if I merge these 2 things.

Thanks,

        Vivien

Thanks,
-Vladimir