netdev - Re: [PATCHv2 net-next 2/4] net: dsa: mv88e6xxx: Monitor and Management tables

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: <87mvgecejs.fsf@ketchup.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Fri, 02 Dec 2016 14:32:39 -0500
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCHv2 net-next 2/4] net: dsa: mv88e6xxx: Monitor and Management tables

Hi Andrew,

Andrew Lunn <andrew@...n.ch> writes:

> @@ -3184,6 +3186,8 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
>  	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
>  	.stats_get_strings = mv88e6095_stats_get_strings,
>  	.stats_get_stats = mv88e6095_stats_get_stats,
> +	.g1_set_cpu_port = mv88e6095_g1_set_cpu_port,
> +	.g1_set_egress_port = mv88e6095_g1_set_egress_port,
>  };

I like the implementation in this version better. But please explain me
why you are prefixing these operations with g1_?

The mv88e6xxx_ops structure is agnostic from the implementation. There
is only one way to implement a feature (e.g. setting the switch MAC) in
Marvell switches. So describing the internal location of the said
feature is wrong and brings no value.

But let's imagine we can set the CPU port in some Global 2 registers.
You are going to wrap this in chip.c with something like:

    int mv88e6xxx_set_cpu_port(struct mv88e6xxx_chip *chip, int port)
    {
        if (chip->info->ops->g2_set_cpu_port)
            return chip->info->ops->g2_set_cpu_port(chip, port);
        else if (chip->info->ops->g1_set_cpu_port)
            return chip->info->ops->g1_set_cpu_port(chip, port);
        else
            return -EOPNOTSUPP;
    }

What's the point of this?

Thanks,

        Vivien