netdev - Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports

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:   Mon, 19 Aug 2019 14:03:47 -0400
From:   Vivien Didelot <vivien.didelot@...il.com>
To:     Marek Behun <marek.behun@....cz>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, f.fainelli@...il.com,
        andrew@...n.ch
Subject: Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports

Hi Marek,

On Mon, 19 Aug 2019 19:32:46 +0200, Marek Behun <marek.behun@....cz> wrote:
> > Call the .port_enable and .port_disable functions for all ports,
> > not only the user ports, so that drivers may optimize the power
> > consumption of all ports after a successful setup.
> > 
> > Unused ports are now disabled on setup. CPU and DSA ports are now
> > enabled on setup and disabled on teardown. User ports were already
> > enabled at slave creation and disabled at slave destruction.
> 
> My original reason for enabling CPU and DSA ports is that enabling
> serdes irq could not be done in .setup in mv88e6xxx, since the required
> phylink structures did not yet exists for those ports.
> 
> The case after this patch would be that .port_enable is called for
> CPU/DSA ports right after these required phylink structures are created
> for this port. A thought came to me while reading this that some driver
> in the future can expect, in their implementation of
> port_enable/port_disable, that phylink structures already exist for all
> ports, not just the one being currently enabled/disabled.
> 
> Wouldn't it be safer if CPU/DSA ports were enabled in setup after all
> ports are registered, and disabled in teardown before ports are
> unregistered?
> 
> Current:
>   ->setup()
>   for each port
>       dsa_port_link_register_of()
>       dsa_port_enable()
> 
> Proposed:
>   ->setup()
>   for each port
>       dsa_port_link_register_of()
>   for each port
>       dsa_port_enable()

I understand what you mean, but the scope of .port_enable really is the
port itself, so I do not expect a driver to configure something else. Also,
I prefer to keep it simple at the moment and not speculate about what a
driver may need in the future, as we may also wonder which switch must be
enabled first in a tree, etc. If that case happens in the future, we can
definitely isolate the ports enabling code after the tree is setup.

So if this series meets your requirement for now, I'd keep it like that.


Thanks,

	Vivien