Re: [PATCH v3 net-next 12/24] net: dsa: Copy the vlan_filtering setting on the CPU port if it's global

[Vladimir Oltean]

On Sat, 13 Apr 2019 at 18:23, Andrew Lunn <andrew@xxxxxxx> wrote:
>
> On Sat, Apr 13, 2019 at 04:28:10AM +0300, Vladimir Oltean wrote:
> > The current behavior is not as obvious as one would assume (which is
> > that, if the driver set vlan_filtering_is_global = 1, then checking any
> > dp->vlan_filtering would yield the same result). Only the ports which
> > are actively enslaved into a bridge would have vlan_filtering set.
> >
> > This makes it tricky for drivers to check what the global state is.
> > Moreover, the most obvious place to check for this setting, the CPU
> > port, is not populated since it's not being enslaved to the bridge.
> > So fix this and make the CPU port hold the global state of VLAN
> > filtering on this switch.
> >
> > Signed-off-by: Vladimir Oltean <olteanv@xxxxxxxxx>
> > ---
> > Changes in v3:
> > Patch is new.
> >
> >  net/dsa/port.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index c8eb2cbcea6e..acb4ed1f9929 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -190,6 +190,8 @@ static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
> >  int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
> >                           struct switchdev_trans *trans)
> >  {
> > +     /* Violate a const pointer here */
> > +     struct dsa_port *cpu_dp = (struct dsa_port *)dp->cpu_dp;
>
> Hi Vladimir
>
> As compilers get more picky, i expect that is going to result in a
> warning.
>
> Since this is a switch global attribute, putting it in dsa_switch
> would be better, next to vlan_filteris_is_global.
>
>         Andrew

Hi Andrew,

Creating a bool ds->vlan_filtering wouldn't make a lot of sense for
the majority of drivers.
Additionally in my sja1105_filter() function, that would require me to
pass through one more pointer (dev->dsa_ptr->vlan_filtering vs
dev->dsa_ptr->*ds->*vlan_filtering) to reach the same information.
I don't think that keeping it in cpu_dp->vlan_filtering has any
semantical overlap with anything else that might appear in the future.
And I don't know why the cpu_dp pointer is const. In the
dsa_switch_tree it isn't.

Thanks,
-Vladimir