netdev - Re: [PATCHv2 net-next 4/4] net: dsa: mv88e6xxx: Refactor CPU and DSA port setup

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] [day] [month] [year] [list]

Date:   Fri, 02 Dec 2016 17:03:30 -0500
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCHv2 net-next 4/4] net: dsa: mv88e6xxx: Refactor CPU and DSA port setup

Hi Andrew,

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

>> The port's EgressMode, FrameMode and EtherType are really tied together
>> to compose the mode of the port.
>
> Setting the EtherType is somewhat separate. It is only needed on ports
> using EDSA. And that can only happen on a CPU port. Humm, actually, i
> set it when i should not. But putting this in a wrapper actually hides
> this.

Wrong. The datasheet says:

    > This Ether Type is used for many features depending upon the mode
    > of the port (as defined by the port’s EgressMode and FrameMode
    > bits – in Port Control, port offset 0x04).

It says that in Normal Network mode, this register can be used to trap,
mirror, etc. Also used in Provider and EDSA modes.

That is why it would be better to wrap them together to ensure correct
values when configuring a port's mode.

>
>> Could you add an helper in chip.c like:
>> 
>>     static int mv88e6xxx_set_port_mode(struct mv88e6xxx_chip *chip, int port,
>>                                        enum mv88e6xxx_frame_mode frame_mode,
>>                                        u16 egress_mode, bool egress_unknown,
>>                                        u16 ethertype)
>>     {
>>         int err;
>> 
>>         if (chip->info->ops->port_set_frame_mode) {
>>             err = chip->info->ops->port_set_frame_mode(chip, port, frame_mode);
>>             if (err)
>>                 return err;
>>         }
>
> Ignoring that it is not implemented here is wrong. It must be
> implemented, or the device is not going to work. It is a question of,
> do we want an oops, or return an error code.

Since that is done at setup time, returning an error is enough IMO to
inform the DSA layer that something went wrong.

Thanks,

        Vivien