Message-ID: <8e908165-9412-57b8-bc30-5b337fdd4033@ti.com>
Date:   Wed, 27 Jun 2018 14:18:00 -0500
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Arnd Bergmann <arnd@...db.de>
CC:     Ivan Vecera <ivecera@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Networking <netdev@...r.kernel.org>,
        <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 06/22/2018 02:45 AM, Ilias Apalodimas wrote:
> 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@...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.
> 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
>>
> 
> If people like this idea, i can send a V3 with these changes.

Nop. I do not think this is good idea, because "dual_mac" mode has very strict
meaning and requirements. In "dual_mac" mode both port should be teated and work
as *separate network devices" (like two, not connected PCI eth cards) - the fact that
it's implemented on top of hw, which can do packet switching doesn't matter here and just a
technical solution.
Main requirements:
1) No packet forwarding is allowed inside hw under any circumstances, only Linux
   Host SW can consume or forward packets
2) One interface should not block another inside CPSW hw which implies special FIFOs/HW
 configuration
As per, above switchdev functionality doesn't make too much sense in "dual_mac" mode and
introducing dual meaning for this mode is not a good choice either.

Again, as discussed, option 4 is considered as preferred.

-- 
regards,
-grygorii

Powered by blists - more mailing lists