netdev - Re: [PATCH v2 1/2] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

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: <20170204173256.GB8364@lunn.ch>
Date:   Sat, 4 Feb 2017 18:32:56 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Lukasz Majewski <lukma@...x.de>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Karicheri Muralidharan <m-karicheri2@...com>,
        linux-kernel@...r.kernel.org, Eric Engestrom <eric@...estrom.ch>,
        netdev@...r.kernel.org, Kishon Vijay Abraham I <kishon@...com>,
        Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [PATCH v2 1/2] net: phy: dp83867: Port mirroring support in the
 DP83867 TI's PHY driver

On Sat, Feb 04, 2017 at 05:02:11PM +0100, Lukasz Majewski wrote:
> This patch adds support for enabling or disabling the port mirroring 
> (at CFG4 register) feature of the DP83867 TI's PHY device.

As we discussed before, "port mirroring" is bad naming. Yes, we should
use it, because that is what the datasheet calls this feature. But the
commit message should also contain a description of what this means,
and reference that the linux name for this concept is lane swapping.

> +enum {

Maybe give the 0 value a name. DP83867_PORT_MIRROING_KEEP?

> +	DP83867_PORT_MIRROING_EN = 1,
> +	DP83867_PORT_MIRROING_DIS,
> +};
> +

That extra enum value can then make this more obvious:

        if (dp83867->port_mirroring != DP83867_PORT_MIRROING_KEEP)
		dp83867_config_port_mirroring(phydev);

On the first reading of the patch, i though you were setting mirroring
on/off under all conditions, but in fact you don't. This makes it
clearer.

	Thanks
		Andrew