| Message ID | 20190116153419.3208-1-miquel.raynal@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: dsa: mv88e6xxx: Add suspend/resume callbacks | expand |
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
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 >
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
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!
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
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.
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
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
> 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
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
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, }, };
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(-)