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

[Marek Behun <marek.behun@nic.cz>]

On Sun, 18 Aug 2019 13:35:45 -0400
Vivien Didelot <vivien.didelot@gmail.com> 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.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

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()

Marek