Message-ID: <20180621124552.GA15208@apalos>
Date:   Thu, 21 Jun 2018 15:45:52 +0300
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Ivan Vecera <ivecera@...hat.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        grygorii.strashko@...com, ivan.khoronzhuk@...aro.org,
        nsekhar@...com, jiri@...nulli.us, francois.ozog@...aro.org,
        yogeshs@...com, spatton@...com, Jose.Abreu@...opsys.com
Subject: Re: [RFC v2, net-next, PATCH 4/4] net/cpsw_switchdev: add switchdev
 mode of operation on cpsw driver

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

Powered by blists - more mailing lists