Made driver wakeup capable, building on suspend and resume handlers. When suspending if wakeup is enabled for the device, its interrupt is flagged for wakeup and the wake interrupt is enabled before the controller is placed into sleep mode.
This appears to work well if the system is allowed to fully enter sleep before a wakeup message arrives. Performed many suspend / resume cycles via can message. Initially very encouraging.
However if the system is suspended while there's traffic on the bus. Tested with 200 msgs/s from another machine. A race condition is exposed. If enabled for wakeup I would expect the controller to either cause the system to abort the suspend process, or suspend and immediately wake up. What happens instead is the system enters suspend and remains there until woken by another source.
Having had a poke around I suspect this is related to placing the controller in sleep mode in the suspend, rather than suspend_noirq stage handler.
When a wake interrupt is registered via enable_irq_wake, after all devices have had suspend called. The wake interrupts are "armed".
Once armed the noirq handlers are called, where the docs hinted that the device should be configured to wake.
It seems that as part of handing an interrupt, irq_may_run is called, which in turn calls irq_pm_check_wakeup, which if the irq is wake enabled will wake the system.
I'd guess that the controller enters suspend and immediately wakes, generating the WAKE interrupt. As interrupts are disabled at the start of suspend (to prevent the handler running after the SPI controller has been suspended) the interrupt doesn't schedule the system to wake as the interrupt isn't "armed" yet. But is somehow marked as pending and isn't re-evaluated after the system finishes suspending.
Unfortunately I don't know all that much about the inner workings of interrupt handling within the kernel.
Let me know what you think / if you're able to give me any pointers of how to solve this one?
Signed-off-by: Phil Greenland <phil@xxxxxxxxxxxxxxx>
---
.../net/can/spi/mcp251xfd/mcp251xfd-core.c | 38 +++++++++++++++++--
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 98eb597d8..bbe7bed9e 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -1072,6 +1072,12 @@ static int mcp251xfd_chip_interrupts_enable(const struct mcp251xfd_priv *priv)
return regmap_write(priv->map_reg, MCP251XFD_REG_INT, val);
}
+static int mcp251xfd_chip_interrupts_enable_suspend(const struct mcp251xfd_priv *priv)
+{
+ /* Enable only WAKE interrupt */
+ return regmap_write(priv->map_reg, MCP251XFD_REG_INT, MCP251XFD_REG_INT_WAKIE);
+}
+
static int mcp251xfd_chip_interrupts_disable(const struct mcp251xfd_priv *priv)
{
int err;
@@ -2814,6 +2820,11 @@ static int mcp251xfd_register(struct mcp251xfd_priv *priv)
if (err)
goto out_chip_set_mode_sleep;
+ device_set_wakeup_capable(&ndev->dev, true);
+
+ if (device_property_read_bool(&ndev->dev, "wakeup-source"))
+ device_set_wakeup_enable(&ndev->dev, true);
+
err = mcp251xfd_register_done(priv);
if (err)
goto out_unregister_candev;
@@ -2851,6 +2862,8 @@ static inline void mcp251xfd_unregister(struct mcp251xfd_priv *priv)
pm_runtime_get_sync(ndev->dev.parent);
pm_runtime_put_noidle(ndev->dev.parent);
+ device_set_wakeup_enable(&ndev->dev, false);
+ device_set_wakeup_capable(&ndev->dev, false);
mcp251xfd_clks_and_vdd_disable(priv);
pm_runtime_disable(ndev->dev.parent);
}
@@ -3074,9 +3087,22 @@ static int __maybe_unused mcp251xfd_suspend(struct device *device)
mcp251xfd_timestamp_stop(priv);
- err = mcp251xfd_chip_interrupts_disable(priv);
- if (err)
- return err;
+ if (device_may_wakeup(&priv->ndev->dev)) {
+ /* Device may wake system from sleep, enable wake on irq */
+ err = enable_irq_wake(priv->ndev->irq);
+ if (err)
+ return err;
+
+ /* Enable (only) wake interrupt */
+ err = mcp251xfd_chip_interrupts_enable_suspend(priv);
+ if (err)
+ return err;
+ } else {
+ /* Device not waking system from sleep, disable all interrupts */
+ err = mcp251xfd_chip_interrupts_disable(priv);
+ if (err)
+ return err;
+ }
err = mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_SLEEP);
if (err)
@@ -3100,6 +3126,12 @@ static int __maybe_unused mcp251xfd_resume(struct device *device)
netif_device_attach(priv->ndev);
netif_start_queue(priv->ndev);
+ if (device_may_wakeup(&priv->ndev->dev)) {
+ err = disable_irq_wake(priv->ndev->irq);
+ if (err)
+ return err;
+ }
+
/* restart oscillator (disabled automatically when entering sleep) */
err = mcp251xfd_chip_clock_enable(priv);
if (err)
--
2.25.1
