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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86d31830-bd53-2800-932b-de610586a5fb@gmail.com>
Date:   Wed, 10 Apr 2019 23:15:21 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>, vivien.didelot@...il.com,
        andrew@...n.ch, davem@...emloft.net
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        georg.waibel@...sor-technik.de
Subject: Re: [PATCH v2 net-next 05/22] net: dsa: Add more convenient functions
 for installing port VLANs

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@...il.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