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

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

[Miquel Raynal]

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.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 52 ++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8a517d8fb9d1..b88efa9b86e0 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -442,15 +442,21 @@ static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
 	return err;
 }
 
+static void mv88e6xxx_do_irq_poll(struct mv88e6xxx_chip *chip)
+{
+	mv88e6xxx_g1_irq_thread_work(chip);
+
+	kthread_queue_delayed_work(chip->kworker, &chip->irq_poll_work,
+				   msecs_to_jiffies(100));
+}
+
 static void mv88e6xxx_irq_poll(struct kthread_work *work)
 {
 	struct mv88e6xxx_chip *chip = container_of(work,
 						   struct mv88e6xxx_chip,
 						   irq_poll_work.work);
-	mv88e6xxx_g1_irq_thread_work(chip);
 
-	kthread_queue_delayed_work(chip->kworker, &chip->irq_poll_work,
-				   msecs_to_jiffies(100));
+	mv88e6xxx_do_irq_poll(chip);
 }
 
 static int mv88e6xxx_irq_poll_setup(struct mv88e6xxx_chip *chip)
@@ -4651,6 +4657,45 @@ static const void *pdata_device_get_match_data(struct device *dev)
 	return NULL;
 }
 
+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);
+}
+
+static int __maybe_unused mv88e6xxx_resume(struct device *dev)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int ret;
+
+	mv88e6xxx_phy_init(chip);
+
+	mutex_lock(&chip->reg_lock);
+	ret = mv88e6xxx_switch_reset(chip);
+	mutex_unlock(&chip->reg_lock);
+	if (ret) {
+		dev_err(dev, "Failed to reset the switch\n");
+		return ret;
+	}
+
+	ret = mv88e6xxx_setup(ds);
+	if (ret) {
+		dev_err(dev, "Failed to setup the switch\n");
+		return ret;
+	}
+
+	mv88e6xxx_do_irq_poll(chip);
+
+	return dsa_switch_resume(ds);
+}
+
+static SIMPLE_DEV_PM_OPS(mv88e6xxx_pm_ops, mv88e6xxx_suspend, mv88e6xxx_resume);
+
 static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct dsa_mv88e6xxx_pdata *pdata = mdiodev->dev.platform_data;
@@ -4835,6 +4880,7 @@ static struct mdio_driver mv88e6xxx_driver = {
 	.mdiodrv.driver = {
 		.name = "mv88e6085",
 		.of_match_table = mv88e6xxx_of_match,
+		.pm = &mv88e6xxx_pm_ops,
 	},
 };
 
-- 
2.19.1

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

[Andrew Lunn]

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:

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

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

[Florian Fainelli]

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

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

[Andrew Lunn]

Hi Florian

> 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.

I expect it is also on Viviens TODO list, since this really could be
in the core.

   Andrew

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

[Vivien Didelot]

Hi,

On Wed, 16 Jan 2019 23:23:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Florian
> 
> > 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.
> 
> I expect it is also on Viviens TODO list, since this really could be
> in the core.

Indeed that is!

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

[Miquel Raynal]

Hi Andrew, Vivien,

Vivien Didelot <vivien.didelot@gmail.com> wrote on Thu, 17 Jan 2019
10:46:41 -0500:

> Hi,
> 
> On Wed, 16 Jan 2019 23:23:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
> > Hi Florian
> >   
> > > 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.  
> > 
> > I expect it is also on Viviens TODO list, since this really could be
> > in the core.  
> 
> Indeed that is!

So, shall I wait for Vivien's patches (adding port_disable/enable()
in dsa_slave_suspend/resume()) and keep the driver as-is or do you want
me to manually call port_disable/enable() from the mv88e6xxx driver?


Thanks,
Miquèl

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

[Florian Fainelli]

On 1/17/19 7:50 AM, Miquel Raynal wrote:
> Hi Andrew, Vivien,
> 
> Vivien Didelot <vivien.didelot@gmail.com> wrote on Thu, 17 Jan 2019
> 10:46:41 -0500:
> 
>> Hi,
>>
>> On Wed, 16 Jan 2019 23:23:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
>>> Hi Florian
>>>   
>>>> 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.  
>>>
>>> I expect it is also on Viviens TODO list, since this really could be
>>> in the core.  
>>
>> Indeed that is!
> 
> So, shall I wait for Vivien's patches (adding port_disable/enable()
> in dsa_slave_suspend/resume()) and keep the driver as-is or do you want
> me to manually call port_disable/enable() from the mv88e6xxx driver?

Up to you guys, the only thing that I an tell you is that my platform
loses its register contents during suspend/resume, therefore you must
make sure the driver re-applies the entire switch configuration,
identical to how it was prior to suspend. If you need me to test
something, please holler.
-- 
Florian

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

[Vivien Didelot]

On Thu, 17 Jan 2019 10:00:46 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>> 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.  
> >>>
> >>> I expect it is also on Viviens TODO list, since this really could be
> >>> in the core.  
> >>
> >> Indeed that is!
> > 
> > So, shall I wait for Vivien's patches (adding port_disable/enable()
> > in dsa_slave_suspend/resume()) and keep the driver as-is or do you want
> > me to manually call port_disable/enable() from the mv88e6xxx driver?
> 
> Up to you guys, the only thing that I an tell you is that my platform
> loses its register contents during suspend/resume, therefore you must
> make sure the driver re-applies the entire switch configuration,
> identical to how it was prior to suspend. If you need me to test
> something, please holler.


Let's wait then.


Thanks,

	Vivien

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

[Miquel Raynal]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> wrote on Thu, 17 Jan 2019
10:00:46 -0800:

> On 1/17/19 7:50 AM, Miquel Raynal wrote:
> > Hi Andrew, Vivien,
> > 
> > Vivien Didelot <vivien.didelot@gmail.com> wrote on Thu, 17 Jan 2019
> > 10:46:41 -0500:
> >   
> >> Hi,
> >>
> >> On Wed, 16 Jan 2019 23:23:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:  
> >>> Hi Florian
> >>>     
> >>>> 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.    
> >>>
> >>> I expect it is also on Viviens TODO list, since this really could be
> >>> in the core.    
> >>
> >> Indeed that is!  
> > 
> > So, shall I wait for Vivien's patches (adding port_disable/enable()
> > in dsa_slave_suspend/resume()) and keep the driver as-is or do you want
> > me to manually call port_disable/enable() from the mv88e6xxx driver?  
> 
> Up to you guys, the only thing that I an tell you is that my platform
> loses its register contents during suspend/resume, therefore you must
> make sure the driver re-applies the entire switch configuration,
> identical to how it was prior to suspend. If you need me to test
> something, please holler.

I am not sure to understand what is lost. On my setup ethtool shows
that everything is fine after resume but maybe I fall into a "default"
working case.

When I compare with the two drivers pointed out by Andrew:
* qca8k resume callback:
	* enable ports,
	* call dsa_switch_resume().
* bcm_sf2 resume callback:
	* call dsa_switch_resume(),
	* reset the switch,
	* refresh rules (Not applicable?),
	* enable the PHYs,
	* setup the ports,
	* configure vlan (Not applicable?),
* mv88e6xxx resume callback:
	* reset the switch,
	* enable the PHYs,
	* setup the ports,
	* enable IRQs,
	* call dsa_switch_resume().

This looks pretty similar. Maybe the ports setup are set to default
values while I should save some parameters at suspend? I changed a
few parameters (like the MTU or the queue length) but they seem to be
correct across suspend cycles. Can you show a diff of what part of the
configuration is lost?

Anyway, I have little background working with switches, so I might miss
something big.


Thanks,
Miquèl

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

[Andrew Lunn]

> I am not sure to understand what is lost. On my setup ethtool shows
> that everything is fine after resume but maybe I fall into a "default"
> working case.

Hi Miquèl

Is the power removed from the switch? If so, you need to restore the
full switch configuration. The current code might be sufficient for
runtime suspend, where the switch is put into a low power mode, but it
is kept powered. It is not sufficient for a full power cycle.

> When I compare with the two drivers pointed out by Andrew:
> * qca8k resume callback:
> 	* enable ports,
> 	* call dsa_switch_resume().
> * bcm_sf2 resume callback:
> 	* call dsa_switch_resume(),
> 	* reset the switch,
> 	* refresh rules (Not applicable?),
> 	* enable the PHYs,
> 	* setup the ports,
> 	* configure vlan (Not applicable?),
> * mv88e6xxx resume callback:
> 	* reset the switch,
> 	* enable the PHYs,
> 	* setup the ports,
> 	* enable IRQs,
> 	* call dsa_switch_resume().
 
Here are some commands to try before you suspend:

ip link add name br0 type bridge
ip link set br0 up
ip link set lan1 up
ip link set lan1 master br0
ip link set lan2 up
ip link set lan2 master br0

At this point, you should be able to pass traffic between lan1 and
lan2.

bridge fdb add 00:42:42:42:42:42 lan1
bridge fdb add 00:24:24:24:24:24 lan2

That adds two static forwarding database entries. You can show these using

bridge fdb show

There are likely to be additional dynamic FDB entries. It is O.K. to
loose the dynamic entries over a suspend/resume cycle, but the static
ones must remain.

> This looks pretty similar. Maybe the ports setup are set to default
> values while I should save some parameters at suspend? I changed a
> few parameters (like the MTU or the queue length) but they seem to be
> correct across suspend cycles.

MTU and queue length have nothing to do with the actual switch. Your
tests need to actually program the hardware.

    Andrew

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

[Miquel Raynal]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> wrote on Tue, 22 Jan 2019 14:20:05 +0100:

> > I am not sure to understand what is lost. On my setup ethtool shows
> > that everything is fine after resume but maybe I fall into a "default"
> > working case.  
> 
> Hi Miquèl
> 
> Is the power removed from the switch? If so, you need to restore the
> full switch configuration. The current code might be sufficient for
> runtime suspend, where the switch is put into a low power mode, but it
> is kept powered. It is not sufficient for a full power cycle.
> 
> > When I compare with the two drivers pointed out by Andrew:
> > * qca8k resume callback:
> > 	* enable ports,
> > 	* call dsa_switch_resume().
> > * bcm_sf2 resume callback:
> > 	* call dsa_switch_resume(),
> > 	* reset the switch,
> > 	* refresh rules (Not applicable?),
> > 	* enable the PHYs,
> > 	* setup the ports,
> > 	* configure vlan (Not applicable?),
> > * mv88e6xxx resume callback:
> > 	* reset the switch,
> > 	* enable the PHYs,
> > 	* setup the ports,
> > 	* enable IRQs,
> > 	* call dsa_switch_resume().  
>  
> Here are some commands to try before you suspend:
> 
> ip link add name br0 type bridge
> ip link set br0 up
> ip link set lan1 up
> ip link set lan1 master br0
> ip link set lan2 up
> ip link set lan2 master br0
> 
> At this point, you should be able to pass traffic between lan1 and
> lan2.
> 
> bridge fdb add 00:42:42:42:42:42 lan1
> bridge fdb add 00:24:24:24:24:24 lan2
> 
> That adds two static forwarding database entries. You can show these using
> 
> bridge fdb show
> 
> There are likely to be additional dynamic FDB entries. It is O.K. to
> loose the dynamic entries over a suspend/resume cycle, but the static
> ones must remain.

Thank you very much for this, it is really helpful.

> 
> > This looks pretty similar. Maybe the ports setup are set to default
> > values while I should save some parameters at suspend? I changed a
> > few parameters (like the MTU or the queue length) but they seem to be
> > correct across suspend cycles.  
> 
> MTU and queue length have nothing to do with the actual switch. Your
> tests need to actually program the hardware.

I didn't checked the hw was programmed indeed. Well, I'll try with the
test described above and will propose something.

Thanks,
Miquèl