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

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@gmail.com>
---
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.

 net/dsa/dsa_priv.h |  2 ++
 net/dsa/port.c     | 31 +++++++++++++++++++++++++++++++
 net/dsa/slave.c    | 16 ++--------------
 3 files changed, 35 insertions(+), 14 deletions(-)

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@gmail.com>
> ---
> 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?

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@gmail.com>
>> ---
>> 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