Hi Vivien,
On Mon, 26 Aug 2019 at 18:20, Vivien Didelot <vivien.dide...@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Sun, 25 Aug 2019 21:44:54 +0300, Vladimir Oltean <olte...@gmail.com> wrote:
> > - 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
