netdev - Re: [PATCH net-next v2 1/2] net: dsa: mv88e6xxx: Split monitor port configuration

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]

Date:   Wed, 23 Oct 2019 03:06:21 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Iwan R Timmer <irtimmer@...il.com>
Cc:     netdev@...r.kernel.org, vivien.didelot@...il.com,
        f.fainelli@...il.com, davem@...emloft.net
Subject: Re: [PATCH net-next v2 1/2] net: dsa: mv88e6xxx: Split monitor port
 configuration

On Mon, Oct 21, 2019 at 11:01:42PM +0200, Iwan R Timmer wrote:
> Separate the configuration of the egress and ingress monitor port.
> This allows the port mirror functionality to do ingress and egress
> port mirroring to separate ports.
> 
> Signed-off-by: Iwan R Timmer <irtimmer@...il.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c    |  9 ++++++++-
>  drivers/net/dsa/mv88e6xxx/chip.h    |  3 ++-
>  drivers/net/dsa/mv88e6xxx/global1.c | 23 +++++++++++------------
>  drivers/net/dsa/mv88e6xxx/global1.h |  6 ++++--
>  4 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 6787d560e9e3..e9735346838d 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2378,7 +2378,14 @@ static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)
>  
>  		if (chip->info->ops->set_egress_port) {
>  			err = chip->info->ops->set_egress_port(chip,
> -							       upstream_port);
> +							       true,
> +							       upstream_port);
> +			if (err)
> +				return err;
> +
> +			err = chip->info->ops->set_egress_port(chip,
> +							       false,
> +							       upstream_port);

Hi Iwam

I never find true/false very simple to understand. Please could you
add an enum. We already have

enum mv88e6xxx_egress_mode {
        MV88E6XXX_EGRESS_MODE_UNMODIFIED,
        MV88E6XXX_EGRESS_MODE_UNTAGGED,
        MV88E6XXX_EGRESS_MODE_TAGGED,
        MV88E6XXX_EGRESS_MODE_ETHERTYPE,
};

so maybe

enum mv88e6xxx_egress_direction {
        MV88E6XXX_EGRESS_DIR_INGRESS,
        MV88E6XXX_EGRESS_DIR_EGRESS,
};

Otherwise the spirit of the patch is O.K.

	  Andrew