Re: [PATCH net-next] net: dsa: mv88e6xxx: Add suspend/resume callbacks

[Florian Fainelli <f.fainelli@gmail.com>]

On 1/16/19 7:56 AM, Andrew Lunn wrote:
> On Wed, Jan 16, 2019 at 04:34:19PM +0100, Miquel Raynal wrote:
>> Bring S2RAM support to the mv88e6xxx DSA driver.
>>
>> The content of the *_irq_poll() helper is moved in *_do_irq_poll() so
>> that that the function can be called from the ->resume() callback
>> without using the *work pointer.
>>
>> +static int __maybe_unused mv88e6xxx_suspend(struct device *dev)
>> +{
>> +	struct dsa_switch *ds = dev_get_drvdata(dev);
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +
>> +	kthread_cancel_delayed_work_sync(&chip->irq_poll_work);
>> +
>> +	return dsa_switch_suspend(ds);
>> +}
> 
> Hi Miquel
> 
> I don't think this is sufficient. You are leaving the switch itself
> running. It will still be forwarding frames. And since the host is
> shutdown, there is nothing doing spanning tree protocol. So you are
> likely to cause loops in your network.
> 
> Take a look at bcm_sf2:

There is a big caveat though, bcm_sf2_port_disable() checks a bitmask of
ports that might have been configured to allow Wake-on-LAN. Technically
you could also leave the switch running during S2RAM on these platforms,
which requires putting it in unmanaged mode, in order to continue having
stations "talk" to each other.

But in general, Andrew, is right, you must quiesce all activity on the
switch while you suspend it.

A possible approach could be to call the port_disable, port_enable
callbacks from dsa_slave_suspend() and dsa_slave_resume(), I might have
some patches doing that already somewhere.

> 
> static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
> {
>         struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>         unsigned int port;
> 
>         bcm_sf2_intr_disable(priv);
> 
>         /* Disable all ports physically present including the IMP
>          * port, the other ones have already been disabled during
>          * bcm_sf2_sw_setup
>          */
>         for (port = 0; port < DSA_MAX_PORTS; port++) {
>                 if (dsa_is_user_port(ds, port) || dsa_is_cpu_port(ds, port))
>                         bcm_sf2_port_disable(ds, port, NULL);
>         }
> 
>         return 0;
> }
> 
> qca8k does something similar.
> 
> 	Andrew
> 


-- 
Florian