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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180618201940.GA5890@apalos>
Date:   Mon, 18 Jun 2018 23:19:40 +0300
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, grygorii.strashko@...com,
        ivan.khoronzhuk@...aro.org, nsekhar@...com, jiri@...nulli.us,
        ivecera@...hat.com, f.fainelli@...il.com, 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 Mon, Jun 18, 2018 at 06:16:27PM +0200, Andrew Lunn wrote:
> > @@ -2711,6 +2789,10 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
> >  	if (of_property_read_bool(node, "dual_emac"))
> >  		data->switch_mode = CPSW_DUAL_EMAC;
> >  
> > +	/* switchdev overrides DTS */
> > +	if (IS_ENABLED(CONFIG_TI_CPSW_SWITCHDEV))
> > +		data->switch_mode = CPSW_SWITCHDEV;
> > +
> 
> I know you discussed this a bit with Jiri, but i still think if
> 'dual_mac" is found, you should do dual mac. The DT clearly requests
> dual mac, and doing anything else is going to cause confusion.
> 
> The device tree binding is ambiguous what should happen when dual-mac
> is missing. So i would only enable swithdev mode in that case.
At the moment if no 'dual_emac;' is found on DTS the driver operates in "switch
mode". It only registers 1 ethernet interface with no ability (unless you patch
the kernel) to configure the switch. If we use DTS instead of a .config option
we should add parsing for something like 'switchdev;' in the DTS.
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.

Ideally i'd prefer something like:
1. Add a DTS option and continue the current behavior. I agree with you that
this will cause less confusion (in fact i prefer it for the current state of the
driver compared to the .config).
or
2. Keep the .config option which is better suited over DTS but might cause some
confusion.
> 
> But ideally, it should be a new driver with a new binding.
TI is better suited to comment on this. The work proposed here is mostly to
accomodate future TSN related configuration for switches. This patchset has
been tested against Ivan's patchset for CBS and is working as expected 
configuration wise.
(http://lkml.iu.edu/hypermail/linux/kernel/1806.1/05302.html)

Thanks
Ilias