From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH 4/4] cpsw: add switchdev support Date: Fri, 1 Jun 2018 14:48:48 -0700 Message-ID: References: <1527144984-31236-1-git-send-email-ilias.apalodimas@linaro.org> <1527144984-31236-5-git-send-email-ilias.apalodimas@linaro.org> <20180524131229.GC24557@lunn.ch> <20180524133234.GA15703@apalos> <20180524163904.GH5128@lunn.ch> <20180525045647.GA1758@apalos> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, grygorii.strashko@ti.com, ivan.khoronzhuk@linaro.org, nsekhar@ti.com, jiri@resnulli.us, ivecera@redhat.com, francois.ozog@linaro.org, yogeshs@ti.com, spatton@ti.com To: Ilias Apalodimas , Andrew Lunn Return-path: Received: from mail-pf0-f196.google.com ([209.85.192.196]:35635 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750934AbeFAVsw (ORCPT ); Fri, 1 Jun 2018 17:48:52 -0400 Received: by mail-pf0-f196.google.com with SMTP id x9-v6so13092264pfm.2 for ; Fri, 01 Jun 2018 14:48:52 -0700 (PDT) In-Reply-To: <20180525045647.GA1758@apalos> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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