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: Thu, 21 Jun 2018 15:45:52 +0300 Message-ID: <20180621124552.GA15208@apalos> References: <1528974690-31600-1-git-send-email-ilias.apalodimas@linaro.org> <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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Fainelli , Andrew Lunn , netdev@vger.kernel.org, grygorii.strashko@ti.com, ivan.khoronzhuk@linaro.org, nsekhar@ti.com, jiri@resnulli.us, francois.ozog@linaro.org, yogeshs@ti.com, spatton@ti.com, Jose.Abreu@synopsys.com To: Ivan Vecera Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:40473 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932879AbeFUMp5 (ORCPT ); Thu, 21 Jun 2018 08:45:57 -0400 Received: by mail-wm0-f67.google.com with SMTP id n5-v6so5791712wmc.5 for ; Thu, 21 Jun 2018 05:45:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: <033dd1bc-0401-a058-5283-4938ac39f701@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Jun 21, 2018 at 02:19:55PM +0200, Ivan Vecera wrote: > On 20.6.2018 20:03, Ilias Apalodimas wrote: > > Hi Florian, > > > > On Wed, Jun 20, 2018 at 10:58:26AM -0700, Florian Fainelli wrote: > >> On 06/20/2018 10:51 AM, Ilias Apalodimas wrote: > >>> Hello Ivan, > >>> On Wed, Jun 20, 2018 at 02:56:48PM +0200, Ivan Vecera wrote: > >>>> On 18.6.2018 22:19, Ilias Apalodimas wrote: > >>>>> Jiri proposed using devlink, which makes sense, but i am not sure it's > >>>>> applicable on this patchset. This will change the driver completely and will > >>>>> totally break backwards compatibility. > >>>> > >>>> Another good reason for a new driver. > >>>> > >>>> I. > >>> This is actually conflicting at least to my understanding. Jiri proposed using > >>> devlink was used as an alternative method to enable a new mode instead of > >>> adding it on a .config option. A new driver wouldn't have a need for that right? > >> > >> Correct, with a new driver would likely behave correctly upon being > >> probed such that you could have your switch ports act as normal network > >> devices from which you could run IP-config and do NFS boot. > > The current driver also does NFS properly and the 2 ethernet ports act as normal > > network interfaces. The NFS section in the cover letter > > is to cover the cases were users running on NFS need to change the running > > switch configuration(starting from adding the 2 interfaces on a bridge). > > Since iproute2 is located on the NFS filesystem the moment > > network connectivity is lost, you loose the ability to perform further > > configuration and in certian configuration scenarios render the device > > unusable. > > Yes, with a new driver you can drop NFS-boot hack you mentioned in cover letter. > All configuration is done during driver probe and thus prior NFS mount. Only > thing you loose with a new driver is backward compatibility but the question is: > DO you really need it? Ok i think there's a bit of confusion here. I'll try to clarify it. There is no NFS hack for the current driver or ever was. Whether you use .config/DTS/devlink/module param method for configuration this is strictly for configuration. The driver (via .config currently) correctly registers and initializes everything it needs to work. NFS boots fine without using anything from that script. The whole script is not there to boot up the device. The script is there to help any potential testing that has to be done *via* NFS and the user has to reconfigure the switch for his testcases. Since you need to add the 2 interfaces in a bridge and start the switch configuration, the moment you do that you loose your network access, thus all the commands you need for configuration. This is why you need to chroot to do that. I don't see how writing a new driver will change that. 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 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. Regards Ilias