Re: [PATCH v2 net-next 05/22] net: dsa: Add more convenient functions for installing port VLANs

[Vladimir Oltean]

On 4/10/19 5:01 AM, Florian Fainelli wrote:
> On 4/9/2019 5:56 PM, Vladimir Oltean wrote:
> > This hides the need to perform a two-phase transaction and construct a
> > switchdev_obj_port_vlan struct.
> >
> > Call graph (including a function that will be introduced in a follow-up
> > patch) looks like this now (same for the *_vlan_del function):
> >
> > dsa_slave_vlan_rx_add_vid   dsa_port_setup_8021q_tagging
> >              |                        |
> >              |                        |
> >              |          +-------------+
> >              |          |
> >              v          v
> >             dsa_port_vid_add      dsa_slave_port_obj_add
> >                    |                         |
> >                    +-------+         +-------+
> >                            |         |
> >                            v         v
> >                         dsa_port_vlan_add
> >
> > Signed-off-by: Vladimir Oltean <olteanv@xxxxxxxxx>
> > ---
> > Changes in v2:
> > Renamed __dsa_port_vlan_add to dsa_port_vid_add and not to
> > dsa_port_vlan_add_trans, as suggested, because the corresponding _del function
> > does not have a transactional phase and the naming is more uniform this way.
>
> Thanks the name does look a lot better now.
>
> [snip]
>
> > +int dsa_port_vid_del(struct dsa_port *dp, u16 vid)
> > +{
> > +	struct switchdev_obj_port_vlan vlan = {
> > +		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> > +		.vid_begin = vid,
> > +		.vid_end = vid,
> > +	};
>
> Any reasons why this function does not take a flags argument? Also, did
> you intend to migrate dsa_slave_vlan_rx_kill_vid() to use this helper
> for deleting a VLAN ID?

Hi Florian,

I don't think there's any semantics associated to deleting a VLAN with 
flags. The br_switchdev_port_vlan_del() function doesn't set the flags 
either.
And as for the dsa_slave_vlan_rx_kill_vid(), it wasn't an oversight, I 
just thought that the benefits weren't as great as in the case of *_add. 
I do see that not doing it somewhat breaks the uniformity and makes the 
commit message slightly inaccurate, so I can fix that up if you think 
it's necessary.

Thanks,
-Vladimir