Date:   Thu, 21 Jun 2018 17:31:31 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc:     Ivan Vecera <ivecera@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Networking <netdev@...r.kernel.org>,
        Grygorii Strashko <grygorii.strashko@...com>,
        ivan.khoronzhuk@...aro.org, Sekhar Nori <nsekhar@...com>,
        Jiří Pírko <jiri@...nulli.us>,
        Francois Ozog <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 2:45 PM, Ilias Apalodimas
<ilias.apalodimas@...aro.org> 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.
* 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.

> 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.

       Arnd

Powered by blists - more mailing lists