From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilias Apalodimas Subject: Re: [RFC v2, net-next, PATCH 4/4] net/cpsw_switchdev: add switchdev mode of operation on cpsw driver Date: Fri, 22 Jun 2018 10:45:32 +0300 Message-ID: <20180622074532.GA27414@apalos> References: <1528974690-31600-5-git-send-email-ilias.apalodimas@linaro.org> <20180618161627.GC5865@lunn.ch> <20180618201940.GA5890@apalos> <671c83ea-6227-cc09-e604-14cfa6804726@redhat.com> <20180620175128.GA27235@apalos> <20180620180342.GA28303@apalos> <033dd1bc-0401-a058-5283-4938ac39f701@redhat.com> <20180621124552.GA15208@apalos> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Ivan Vecera , Florian Fainelli , Andrew Lunn , Networking , Grygorii Strashko , ivan.khoronzhuk@linaro.org, Sekhar Nori , =?utf-8?B?SmnFmcOtIFDDrXJrbw==?= , Francois Ozog , yogeshs@ti.com, spatton@ti.com, Jose.Abreu@synopsys.com To: Arnd Bergmann Return-path: Received: from mail-wm0-f65.google.com ([74.125.82.65]:50247 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750866AbeFVHpi (ORCPT ); Fri, 22 Jun 2018 03:45:38 -0400 Received: by mail-wm0-f65.google.com with SMTP id e16-v6so1204822wmd.0 for ; Fri, 22 Jun 2018 00:45:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Jun 21, 2018 at 05:31:31PM +0200, Arnd Bergmann wrote: > On Thu, Jun 21, 2018 at 2:45 PM, Ilias Apalodimas > wrote: > > On Thu, Jun 21, 2018 at 02:19:55PM +0200, Ivan Vecera wrote: > > > The driver is currently widely used and that's the reason we tried to avoid > > rewriting it. The current driver uses a DTS option to distinguish between two > > existing modes. This patch just adds a third one. So to my understanding we > > have the following options: > > 1. The driver already uses DTS to configure the hardware mode. Although this is > > techincally wrong, we can add a third mode on DTS called 'switchdev;', get rid > > of the .config option and keep the configuration method common (although not > > optimal). > > 2. Keep the .config option which overrides the 2 existing modes. > > 3. Introduce a devlink option. If this is applied for all 3 modes, it will break > > backwards compatibility, so it's not an option. If it's introduced for > > configuring 'switchdev' mode only, we fall into the same pitfall as option 2), > > we have something that overrides our current config, slightly better though > > since it's not a compile time option. > > 4. rewrite the driver > > As I understand it, the switchdev support can also be added without > becoming incompatible with the existing behavior, this is how I would > suggest it gets added in a way that keeps the existing DT binding and > user view while adding switchdev: > > * In non-"dual-emac" mode, show only one network device that is > configured as a transparent switch as today. Any users that today > add TI's third-party ioctl interface with a non-upstreamable patch > can keep using this mode and try to forward-port that patch. Correct > * In "dual-emac" mode (as selected through DT), the hardware is > configured to be viewed as two separate network devices as before, > regardless of kernel configuration. Users can add the two device > to a bridge device as before, and enabling switchdev support in > the kernel configuration (based on your patch series) would change > nothing else than using hardware support in the background to > reconfigure the HW VLAN settings. > > This does not require using devlink, adding a third mode, or changing > the DT binding or the user-visible behavior when switchdev is enabled, > but should get all the features you want. > Correct again. This is doable and the changes on the current patchset are somewhat trivial (detecting a bridge and making the configuration changes on the fly). > > If it was a brand new driver, i'd definitely pick 4. Since it's a pre-existing > > driver though i can't rule out the rest of the options. > > I think the suggestion was to have a new driver with a new binding > so that the DT could choose between the two drivers, one with > somewhat obscure behavior and the other with proper behavior. > > However, from what I can tell, the only requirement to get a somewhat > reasonable behavior is that you enable "dual-emac" mode in DT > to get switchdev support. It would be trivial to add a new compatible > value that only allows that mode along with supporting switchdev, > but I don't think that's necessary here. > > Writing a new driver might also be a good idea (depending on the > quality of the existing one, I haven't looked in detail), but again > I would see no reason for the new driver to be incompatible with > the existing binding, so a gradual cleanup seems like a better > approach. Agree > > Arnd If people like this idea, i can send a V3 with these changes. Thanks Ilias