netdev - Re: [PATCH 4/4] cpsw: add switchdev support

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:   Fri, 1 Jun 2018 14:48:48 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, grygorii.strashko@...com,
        ivan.khoronzhuk@...aro.org, nsekhar@...com, jiri@...nulli.us,
        ivecera@...hat.com, francois.ozog@...aro.org, yogeshs@...com,
        spatton@...com
Subject: Re: [PATCH 4/4] cpsw: add switchdev support



On 05/24/2018 09:56 PM, Ilias Apalodimas wrote:
> On Thu, May 24, 2018 at 06:39:04PM +0200, Andrew Lunn wrote:
>> On Thu, May 24, 2018 at 04:32:34PM +0300, Ilias Apalodimas wrote:
>>> On Thu, May 24, 2018 at 03:12:29PM +0200, Andrew Lunn wrote:
>>>> Device tree is supposed to describe the hardware. Using that hardware
>>>> in different ways is not something you should describe in DT.
>>>>
>>> The new switchdev mode is applied with a .config option in the kernel. What you
>>> see is pre-existing code, so i am not sure if i should change it in this
>>> patchset.
>>
>> If you break the code up into a library and two drivers, it becomes a
>> moot point.
> Agree
> 
>>
>> But what i don't like here is that the device tree says to do dual
>> mac. But you ignore that and do sometime else. I would prefer that if
>> DT says dual mac, and switchdev is compiled in, the probe fails with
>> EINVAL. Rather than ignore something, make it clear it is invalid.
> The switch has 3 modes of operation as is.
> 1. switch mode, to enable that you don't need to add anything on
> the DTS and linux registers a single netdev interface.
> 2. dual mac mode, this is when you need to add dual_emac; on the DTS.
> 3. switchdev mode which is controlled by a .config option, since as you 
> pointed out DTS was not made for controlling config options. 
> 
> I agree that this is far from beautiful. If the driver remains as in though,
> i'd prefer either keeping what's there or making "switchdev" a DTS option, 
> following the pre-existing erroneous usage rather than making the device 
> unusable.  If we end up returning some error and refuse to initialize, users 
> that remote upgrade their equipment, without taking a good look at changelog,
> will loose access to their devices with no means of remotely fixing that.

It seems to me that the mistake here is seeing multiple modes of
operations for the cpsw. There are not actually many, there is one
usage, and then there is what you can and cannot offload. The basic
premise with switchdev and DSA (which uses switchdev) is that each
user-facing port of your switch needs to work as if it were a normal
Ethernet NIC, that is what you call dual-MAC I believe. Then, when you
create a bridge and you enslave those ports into the bridge, you need to
have forwarding done in hardware between these two ports when the
SMAC/DMAC are not for the host/CPU/management interface and you must
simultaneously still have the host have the ability to send/receive
traffic through the bridge device.

It seems to me like this is entirely doable given that the dual MAC use
case is supported already.

switchdev is just a stateless framework to get notified from the
networking stack about what you can possibly offload in hardware, so
having a DTS option gate that is unfortunately wrong because it is
really implementing a SW policy in DTS which is not what it is meant for.
-- 
Florian