netdev - Re: [net-next PATCH 2/2] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <6757c233-b072-837b-e1ce-f88d8819b396@gmail.com>
Date:   Sun, 23 Aug 2020 19:06:10 -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] net: dsa: rtl8366: Refactor VLAN/PVID init



On 8/23/2020 1:59 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>
> ---

[snip]
> +/**
> + * rtl8366_obtain_mc() - retrieve or allocate a VLAN member configuration
> + * @smi: the Realtek SMI device instance
> + * @vid: the VLAN ID to look up or allocate
> + * @vlanmc: the pointer will be assigned to a pointer to a valid member config
> + * if successful
> + * @index: the pointer will be assigned to the index of the valid member config
> + * if successful
> + * @return: 0 or error number

You could return the index directly, since only negative error codes are 
of interest to deal with failures?

[snip]

> +
> +	dev_err(smi->dev, "all VLAN member configurations are in use\n");
> +	return -EINVAL;

Maybe -ENOSPC?

[snip]

   				vid);
> +
> +		ret = rtl8366_set_pvid(smi, port, vid);
> +		if (ret)
> +			dev_err(smi->dev,
> +				"failed to set PVID on port %d to VLAN %04x",
> +				port, vid);
> +
> +		if (!ret)
> +			dev_dbg(smi->dev, "VLAN add: added VLAN %04x with PVID on port %d\n",
> +				vid, port);

Since you are changing this message to a debug print, can we bring some 
consistency along the way and print the VID as decimal (not hexadecimal) 
to match the prior instances?
-- 
Florian