[PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[Miquel Raynal]

On one hand, the mv88e6xxx driver has a work queue called in loop
which will attempt register accesses after MDIO bus suspension, that
entirely freezes the platform during suspend.

On the other hand, the DSA core is not ready yet to support suspend to
RAM operation because so far there is no way to recover reliably the
switch configuration.

To avoid the kernel to freeze when suspending with a switch driven by
the mv88e6xxx driver, we choose to prevent the driver suspension and
in the same way, the whole platform.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Changes since v1/v2
===================
* After having discussed with Vivien and Andrew, it seems that
  preventing the mv88e6xxx driver to suspend is the best option we
  have as of today. I removed the code saving the switch rules at
  driver level, I forgot about saving them at DSA level, so here are
  just two dummy PM hooks that prevent suspension.
  For people interested in picking-up what has been written and
  continue working on it, my initial proposal is available there:
  https://lore.kernel.org/netdev/20190130104606.31639abb@xps13/T/


 drivers/net/dsa/mv88e6xxx/chip.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8a517d8fb9d1..28764d6d7388 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4651,6 +4651,21 @@ static const void *pdata_device_get_match_data(struct device *dev)
 	return NULL;
 }
 
+/* There is no suspend to RAM support at DSA level yet, the switch configuration
+ * would be lost after a power cycle so prevent it to be suspended.
+ */
+static int __maybe_unused mv88e6xxx_suspend(struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
+
+static int __maybe_unused mv88e6xxx_resume(struct device *dev)
+{
+	return 0;
+}
+
+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 +4850,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 v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[Andrew Lunn]

On Tue, Feb 05, 2019 at 12:07:28PM +0100, Miquel Raynal wrote:
> On one hand, the mv88e6xxx driver has a work queue called in loop
> which will attempt register accesses after MDIO bus suspension, that
> entirely freezes the platform during suspend.
> 
> On the other hand, the DSA core is not ready yet to support suspend to
> RAM operation because so far there is no way to recover reliably the
> switch configuration.
> 
> To avoid the kernel to freeze when suspending with a switch driven by
> the mv88e6xxx driver, we choose to prevent the driver suspension and
> in the same way, the whole platform.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[Vivien Didelot]

Hi Miquel,

On Tue,  5 Feb 2019 12:07:28 +0100, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> +/* There is no suspend to RAM support at DSA level yet, the switch configuration
> + * would be lost after a power cycle so prevent it to be suspended.
> + */
> +static int __maybe_unused mv88e6xxx_suspend(struct device *dev)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int __maybe_unused mv88e6xxx_resume(struct device *dev)
> +{
> +	return 0;
> +}

The code looks good but my only concern is -EOPNOTSUPP. In this
context this code is specific to callbacks targeting bridge and
switchdev, while the dev_pm_ops are completely parallel to DSA.

It is intuitive but given Documentation/power/runtime_pm.txt, this
will default to being interpreted as a fatal error, while -EBUSY
seems to keep the device in an 'active' state in a saner way.

I don't understand yet how to properly tell PM core that suspend to RAM
isn't supported. If an error code different from -EAGAIN or -EBUSY
is the way to go, I'm good with it:

Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>


Thanks,

	Vivien

Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[Miquel Raynal]

Hi Vivien,

Vivien Didelot <vivien.didelot@gmail.com> wrote on Tue, 5 Feb 2019
11:28:57 -0500:

> Hi Miquel,
> 
> On Tue,  5 Feb 2019 12:07:28 +0100, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > +/* There is no suspend to RAM support at DSA level yet, the switch configuration
> > + * would be lost after a power cycle so prevent it to be suspended.
> > + */
> > +static int __maybe_unused mv88e6xxx_suspend(struct device *dev)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int __maybe_unused mv88e6xxx_resume(struct device *dev)
> > +{
> > +	return 0;
> > +}  
> 
> The code looks good but my only concern is -EOPNOTSUPP. In this
> context this code is specific to callbacks targeting bridge and
> switchdev, while the dev_pm_ops are completely parallel to DSA.
> 
> It is intuitive but given Documentation/power/runtime_pm.txt, this
> will default to being interpreted as a fatal error, while -EBUSY
> seems to keep the device in an 'active' state in a saner way.
> 
> I don't understand yet how to properly tell PM core that suspend to RAM
> isn't supported. If an error code different from -EAGAIN or -EBUSY
> is the way to go, I'm good with it:

I do share your concern and I went through the Documentation but I did
not find a unified way to tell the PM core the feature is unsupported.

By grepping code, I realized returning -EOPNOTSUPP was a recurrent
alternative so here we are. I also considered -EBUSY but it seems more
like a "I cannot right now" and -EAGAIN which is more a "try again
soon". Anyway, no matter the error code returned, I'm not sure if the PM
core actually cares?
 
> Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
> 
> 
> Thanks,
> 
> 	Vivien


Thanks,
Miquèl

Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[David Miller]

From: Miquel Raynal <miquel.raynal@bootlin.com>
Date: Tue,  5 Feb 2019 12:07:28 +0100

> On one hand, the mv88e6xxx driver has a work queue called in loop
> which will attempt register accesses after MDIO bus suspension, that
> entirely freezes the platform during suspend.
> 
> On the other hand, the DSA core is not ready yet to support suspend to
> RAM operation because so far there is no way to recover reliably the
> switch configuration.
> 
> To avoid the kernel to freeze when suspending with a switch driven by
> the mv88e6xxx driver, we choose to prevent the driver suspension and
> in the same way, the whole platform.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to net-next, thanks.

Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On Tue 2019-02-05 12:07:28, Miquel Raynal wrote:
> On one hand, the mv88e6xxx driver has a work queue called in loop
> which will attempt register accesses after MDIO bus suspension, that
> entirely freezes the platform during suspend.
> 
> On the other hand, the DSA core is not ready yet to support suspend to
> RAM operation because so far there is no way to recover reliably the
> switch configuration.
> 
> To avoid the kernel to freeze when suspending with a switch driven by
> the mv88e6xxx driver, we choose to prevent the driver suspension and
> in the same way, the whole platform.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Could we at least do printk() so that user knows what went wrong?

Debugging s2ram is usually not easy :-(.
								Pavel
 
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[Miquel Raynal]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

Hi Pavel,

Pavel Machek <pavel@ucw.cz> wrote on Mon, 8 Apr 2019 23:55:41 +0200:

> On Tue 2019-02-05 12:07:28, Miquel Raynal wrote:
> > On one hand, the mv88e6xxx driver has a work queue called in loop
> > which will attempt register accesses after MDIO bus suspension, that
> > entirely freezes the platform during suspend.
> > 
> > On the other hand, the DSA core is not ready yet to support suspend to
> > RAM operation because so far there is no way to recover reliably the
> > switch configuration.
> > 
> > To avoid the kernel to freeze when suspending with a switch driven by
> > the mv88e6xxx driver, we choose to prevent the driver suspension and
> > in the same way, the whole platform.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Could we at least do printk() so that user knows what went wrong?
> 
> Debugging s2ram is usually not easy :-(.

I suppose you will be told that suspend was refused by a driver
(probably without stating which one though). You may send a patch to add
a trace if you think it is important, as this change as already been
merged.


Thanks,
Miquèl

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Prevent suspend to RAM

[Andrew Lunn]

On Tue, Apr 09, 2019 at 09:06:43AM +0200, Miquel Raynal wrote:
> Hi Pavel,
> 
> Pavel Machek <pavel@ucw.cz> wrote on Mon, 8 Apr 2019 23:55:41 +0200:
> 
> > On Tue 2019-02-05 12:07:28, Miquel Raynal wrote:
> > > On one hand, the mv88e6xxx driver has a work queue called in loop
> > > which will attempt register accesses after MDIO bus suspension, that
> > > entirely freezes the platform during suspend.
> > > 
> > > On the other hand, the DSA core is not ready yet to support suspend to
> > > RAM operation because so far there is no way to recover reliably the
> > > switch configuration.
> > > 
> > > To avoid the kernel to freeze when suspending with a switch driven by
> > > the mv88e6xxx driver, we choose to prevent the driver suspension and
> > > in the same way, the whole platform.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > 
> > Could we at least do printk() so that user knows what went wrong?
> > 
> > Debugging s2ram is usually not easy :-(.
> 
> I suppose you will be told that suspend was refused by a driver
> (probably without stating which one though). You may send a patch to add
> a trace if you think it is important, as this change as already been
> merged.

Hi Miquel, Pavel

I don't know the s2ram code at all, but this seems like a generic
problem. The core should be reporting which driver returned an error,
not the driver itself.

    Andrew