netdev - Re: [net-next PATCH 2/2 v2] net: dsa: rtl8366: Refactor VLAN/PVID init

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: <15cda1ef-c145-990b-5318-eac70338c702@gmail.com>
Date:   Tue, 1 Sep 2020 12:14:37 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>
Subject: Re: [net-next PATCH 2/2 v2] net: dsa: rtl8366: Refactor VLAN/PVID
 init



On 9/1/2020 12:08 PM, Linus Walleij wrote:
> The VLANs and PVIDs on the RTL8366 utilizes a "member
> configuration" (MC) which is largely unexplained in the
> code.
> 
> This set-up requires a special ordering: rtl8366_set_pvid()
> must be called first, followed by rtl8366_set_vlan(),
> else the MC will not be properly allocated. Relax this
> by factoring out the code obtaining an MC and reuse
> the helper in both rtl8366_set_pvid() and
> rtl8366_set_vlan() so we remove this strict ordering
> requirement.
> 
> In the process, add some better comments and debug prints
> so people who read the code understand what is going on.
> 
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>

One question below:

[snip]

> +	/* Update the MC entry */
> +	vlanmc.member |= member;
> +	vlanmc.untag |= untag;
> +	vlanmc.fid = fid;

Is not there a problem with rtl8366_vlan_del() which is clearing the 
entire vlanmc structure when it it should be doing:

	vlanmc.member &= ~BIT(port);
	vlanmc.untag &= ~BIT(port);

or something along these lines?
-- 
Florian