Re: [RFC v2, net-next, PATCH 4/4] net/cpsw_switchdev: add switchdev mode of operation on cpsw driver

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Thu, Jun 21, 2018 at 05:31:31PM +0200, Arnd Bergmann wrote:
> On Thu, Jun 21, 2018 at 2:45 PM, Ilias Apalodimas
> <ilias.apalodimas@linaro.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.
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