netdev - Re: [PATCH 4/4] cpsw: add switchdev support

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]

Date:   Fri, 25 May 2018 07:56:47 +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, francois.ozog@...aro.org, yogeshs@...com,
        spatton@...com
Subject: Re: [PATCH 4/4] cpsw: add switchdev support

On Thu, May 24, 2018 at 06:39:04PM +0200, Andrew Lunn wrote:
> On Thu, May 24, 2018 at 04:32:34PM +0300, Ilias Apalodimas wrote:
> > On Thu, May 24, 2018 at 03:12:29PM +0200, Andrew Lunn wrote:
> > > Device tree is supposed to describe the hardware. Using that hardware
> > > in different ways is not something you should describe in DT.
> > > 
> > The new switchdev mode is applied with a .config option in the kernel. What you
> > see is pre-existing code, so i am not sure if i should change it in this
> > patchset.
> 
> If you break the code up into a library and two drivers, it becomes a
> moot point.
Agree

> 
> But what i don't like here is that the device tree says to do dual
> mac. But you ignore that and do sometime else. I would prefer that if
> DT says dual mac, and switchdev is compiled in, the probe fails with
> EINVAL. Rather than ignore something, make it clear it is invalid.
The switch has 3 modes of operation as is.
1. switch mode, to enable that you don't need to add anything on
the DTS and linux registers a single netdev interface.
2. dual mac mode, this is when you need to add dual_emac; on the DTS.
3. switchdev mode which is controlled by a .config option, since as you 
pointed out DTS was not made for controlling config options. 

I agree that this is far from beautiful. If the driver remains as in though,
i'd prefer either keeping what's there or making "switchdev" a DTS option, 
following the pre-existing erroneous usage rather than making the device 
unusable.  If we end up returning some error and refuse to initialize, users 
that remote upgrade their equipment, without taking a good look at changelog,
will loose access to their devices with no means of remotely fixing that.

Regards
Ilias