[PATCH V4 1/2] can: flexcan: fix deadlock when using self wakeup

[PATCH V4 1/2] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang]

As reproted by Sean Nyekjaer below:
When suspending, when there is still can traffic on the interfaces the
flexcan immediately wakes the platform again. As it should :-). But it
throws this error msg:
[ 3169.378661] PM: noirq suspend of devices failed

On the way down to suspend the interface that throws the error message does
call flexcan_suspend but fails to call flexcan_noirq_suspend. That means the
flexcan_enter_stop_mode is called, but on the way out of suspend the driver
only calls flexcan_resume and skips flexcan_noirq_resume, thus it doesn't call
flexcan_exit_stop_mode. This leaves the flexcan in stop mode, and with the
current driver it can't recover from this even with a soft reboot, it requires
a hard reboot.

The best way to exit stop mode is in Wake Up interrupt context, and then
suspend() and resume() functions can be symmetric. However, stop mode
request and ack will be controlled by SCU(System Control Unit) firmware(manage
clock,power,stop mode, etc. by Cortex-M4 core) in coming i.MX8(QM/QXP). And SCU
firmware interface can't be available in interrupt context.

For compatibillity, the wake up mechanism can't be symmetric, so we need
in_stop_mode hack.

Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Reported-by: Sean Nyekjaer <sean@geanix.com>
Tested-by: Sean Nyekjaer <sean@geanix.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Changelog:
V1->V2:
	* add Reported-by tag.
	* rebase on patch: can:flexcan:fix stop mode acknowledgment.
V2->V3:
	* rebase on linux-can/testing.
	* change into patch set.
V3->V4:
	* add Tested-by tag.
---
 drivers/net/can/flexcan.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 1cd5179cb876..24cc386c4bce 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -286,6 +286,7 @@ struct flexcan_priv {
 	const struct flexcan_devtype_data *devtype_data;
 	struct regulator *reg_xceiver;
 	struct flexcan_stop_mode stm;
+	bool in_stop_mode;
 
 	/* Read and Write APIs */
 	u32 (*read)(void __iomem *addr);
@@ -1670,6 +1671,8 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 			err = flexcan_enter_stop_mode(priv);
 			if (err)
 				return err;
+
+			priv->in_stop_mode = true;
 		} else {
 			err = flexcan_chip_disable(priv);
 			if (err)
@@ -1696,6 +1699,15 @@ static int __maybe_unused flexcan_resume(struct device *device)
 		netif_device_attach(dev);
 		netif_start_queue(dev);
 		if (device_may_wakeup(device)) {
+			if (priv->in_stop_mode) {
+				flexcan_enable_wakeup_irq(priv, false);
+				err = flexcan_exit_stop_mode(priv);
+				if (err)
+					return err;
+
+				priv->in_stop_mode = false;
+			}
+
 			disable_irq_wake(dev->irq);
 		} else {
 			err = pm_runtime_force_resume(device);
@@ -1732,6 +1744,11 @@ static int __maybe_unused flexcan_noirq_suspend(struct device *device)
 	struct net_device *dev = dev_get_drvdata(device);
 	struct flexcan_priv *priv = netdev_priv(dev);
 
+	/* Need to enable wakeup interrupt in noirq suspend stage. Otherwise,
+	 * it will trigger continuously wakeup interrupt if the wakeup event
+	 * comes before noirq suspend stage, and simultaneously it has enter
+	 * the stop mode.
+	 */
 	if (netif_running(dev) && device_may_wakeup(device))
 		flexcan_enable_wakeup_irq(priv, true);
 
@@ -1744,11 +1761,17 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device)
 	struct flexcan_priv *priv = netdev_priv(dev);
 	int err;
 
+	/* Need to exit stop mode in noirq resume stage. Otherwise, it will
+	 * trigger continuously wakeup interrupt if the wakeup event comes,
+	 * and simultaneously it has still in stop mode.
+	 */
 	if (netif_running(dev) && device_may_wakeup(device)) {
 		flexcan_enable_wakeup_irq(priv, false);
 		err = flexcan_exit_stop_mode(priv);
 		if (err)
 			return err;
+
+		priv->in_stop_mode = false;
 	}
 
 	return 0;
-- 
2.17.1

[PATCH V4 2/2] can: flexcan: add LPSR mode support for i.MX7D

[Joakim Zhang]

For i.MX7D LPSR mode, the controller will lost power and got the
configuration state lost after system resume back. (coming i.MX8QM/QXP
will also completely power off the domain, the controller state will be
lost and needs restore).
So we need to set pinctrl state again and re-start chip to do
re-configuration after resume.

For wakeup case, it should not set pinctrl to sleep state by
pinctrl_pm_select_sleep_state.
For interface is not up before suspend case, we don't need
re-configure as it will be configured by user later by interface up.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

ChangeLog:
V2->V3:
	* rebase on linux-can/testing.
	* change into patch set.
V3->V4:
	* rebase on linux-can/testing.
---
 drivers/net/can/flexcan.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 24cc386c4bce..e2bc5078615f 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -26,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/regmap.h>
 
 #define DRV_NAME			"flexcan"
@@ -1660,7 +1661,7 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct flexcan_priv *priv = netdev_priv(dev);
-	int err = 0;
+	int err;
 
 	if (netif_running(dev)) {
 		/* if wakeup is enabled, enter stop mode
@@ -1674,25 +1675,27 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 
 			priv->in_stop_mode = true;
 		} else {
-			err = flexcan_chip_disable(priv);
+			flexcan_chip_stop(dev);
+
+			err = pm_runtime_force_suspend(device);
 			if (err)
 				return err;
 
-			err = pm_runtime_force_suspend(device);
+			pinctrl_pm_select_sleep_state(device);
 		}
 		netif_stop_queue(dev);
 		netif_device_detach(dev);
 	}
 	priv->can.state = CAN_STATE_SLEEPING;
 
-	return err;
+	return 0;
 }
 
 static int __maybe_unused flexcan_resume(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct flexcan_priv *priv = netdev_priv(dev);
-	int err = 0;
+	int err;
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 	if (netif_running(dev)) {
@@ -1710,15 +1713,19 @@ static int __maybe_unused flexcan_resume(struct device *device)
 
 			disable_irq_wake(dev->irq);
 		} else {
+			pinctrl_pm_select_default_state(device);
+
 			err = pm_runtime_force_resume(device);
 			if (err)
 				return err;
 
-			err = flexcan_chip_enable(priv);
+			err = flexcan_chip_start(dev);
+			if (err)
+				return err;
 		}
 	}
 
-	return err;
+	return 0;
 }
 
 static int __maybe_unused flexcan_runtime_suspend(struct device *device)
-- 
2.17.1

RE: [PATCH V4 1/2] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang]

Kindly Ping...

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2019年10月9日 16:13
> To: mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org; sean@geanix.com;
> dl-linux-imx <linux-imx@nxp.com>; Joakim Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH V4 1/2] can: flexcan: fix deadlock when using self wakeup
> 
> As reproted by Sean Nyekjaer below:
> When suspending, when there is still can traffic on the interfaces the flexcan
> immediately wakes the platform again. As it should :-). But it throws this error
> msg:
> [ 3169.378661] PM: noirq suspend of devices failed
> 
> On the way down to suspend the interface that throws the error message does
> call flexcan_suspend but fails to call flexcan_noirq_suspend. That means the
> flexcan_enter_stop_mode is called, but on the way out of suspend the driver
> only calls flexcan_resume and skips flexcan_noirq_resume, thus it doesn't call
> flexcan_exit_stop_mode. This leaves the flexcan in stop mode, and with the
> current driver it can't recover from this even with a soft reboot, it requires a
> hard reboot.
> 
> The best way to exit stop mode is in Wake Up interrupt context, and then
> suspend() and resume() functions can be symmetric. However, stop mode
> request and ack will be controlled by SCU(System Control Unit)
> firmware(manage clock,power,stop mode, etc. by Cortex-M4 core) in coming
> i.MX8(QM/QXP). And SCU firmware interface can't be available in interrupt
> context.
> 
> For compatibillity, the wake up mechanism can't be symmetric, so we need
> in_stop_mode hack.
> 
> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> Reported-by: Sean Nyekjaer <sean@geanix.com>
> Tested-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> 
> Changelog:
> V1->V2:
> 	* add Reported-by tag.
> 	* rebase on patch: can:flexcan:fix stop mode acknowledgment.
> V2->V3:
> 	* rebase on linux-can/testing.
> 	* change into patch set.
> V3->V4:
> 	* add Tested-by tag.
> ---
>  drivers/net/can/flexcan.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> 1cd5179cb876..24cc386c4bce 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -286,6 +286,7 @@ struct flexcan_priv {
>  	const struct flexcan_devtype_data *devtype_data;
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
> +	bool in_stop_mode;
> 
>  	/* Read and Write APIs */
>  	u32 (*read)(void __iomem *addr);
> @@ -1670,6 +1671,8 @@ static int __maybe_unused flexcan_suspend(struct
> device *device)
>  			err = flexcan_enter_stop_mode(priv);
>  			if (err)
>  				return err;
> +
> +			priv->in_stop_mode = true;
>  		} else {
>  			err = flexcan_chip_disable(priv);
>  			if (err)
> @@ -1696,6 +1699,15 @@ static int __maybe_unused flexcan_resume(struct
> device *device)
>  		netif_device_attach(dev);
>  		netif_start_queue(dev);
>  		if (device_may_wakeup(device)) {
> +			if (priv->in_stop_mode) {
> +				flexcan_enable_wakeup_irq(priv, false);
> +				err = flexcan_exit_stop_mode(priv);
> +				if (err)
> +					return err;
> +
> +				priv->in_stop_mode = false;
> +			}
> +
>  			disable_irq_wake(dev->irq);
>  		} else {
>  			err = pm_runtime_force_resume(device); @@ -1732,6 +1744,11
> @@ static int __maybe_unused flexcan_noirq_suspend(struct device *device)
>  	struct net_device *dev = dev_get_drvdata(device);
>  	struct flexcan_priv *priv = netdev_priv(dev);
> 
> +	/* Need to enable wakeup interrupt in noirq suspend stage. Otherwise,
> +	 * it will trigger continuously wakeup interrupt if the wakeup event
> +	 * comes before noirq suspend stage, and simultaneously it has enter
> +	 * the stop mode.
> +	 */
>  	if (netif_running(dev) && device_may_wakeup(device))
>  		flexcan_enable_wakeup_irq(priv, true);
> 
> @@ -1744,11 +1761,17 @@ static int __maybe_unused
> flexcan_noirq_resume(struct device *device)
>  	struct flexcan_priv *priv = netdev_priv(dev);
>  	int err;
> 
> +	/* Need to exit stop mode in noirq resume stage. Otherwise, it will
> +	 * trigger continuously wakeup interrupt if the wakeup event comes,
> +	 * and simultaneously it has still in stop mode.
> +	 */
>  	if (netif_running(dev) && device_may_wakeup(device)) {
>  		flexcan_enable_wakeup_irq(priv, false);
>  		err = flexcan_exit_stop_mode(priv);
>  		if (err)
>  			return err;
> +
> +		priv->in_stop_mode = false;
>  	}
> 
>  	return 0;
> --
> 2.17.1

RE: [PATCH V4 1/2] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang]

Kindly Ping...

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2019年10月9日 16:13
> To: mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org; sean@geanix.com;
> dl-linux-imx <linux-imx@nxp.com>; Joakim Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH V4 1/2] can: flexcan: fix deadlock when using self wakeup
> 
> As reproted by Sean Nyekjaer below:
> When suspending, when there is still can traffic on the interfaces the flexcan
> immediately wakes the platform again. As it should :-). But it throws this error
> msg:
> [ 3169.378661] PM: noirq suspend of devices failed
> 
> On the way down to suspend the interface that throws the error message does
> call flexcan_suspend but fails to call flexcan_noirq_suspend. That means the
> flexcan_enter_stop_mode is called, but on the way out of suspend the driver
> only calls flexcan_resume and skips flexcan_noirq_resume, thus it doesn't call
> flexcan_exit_stop_mode. This leaves the flexcan in stop mode, and with the
> current driver it can't recover from this even with a soft reboot, it requires a
> hard reboot.
> 
> The best way to exit stop mode is in Wake Up interrupt context, and then
> suspend() and resume() functions can be symmetric. However, stop mode
> request and ack will be controlled by SCU(System Control Unit)
> firmware(manage clock,power,stop mode, etc. by Cortex-M4 core) in coming
> i.MX8(QM/QXP). And SCU firmware interface can't be available in interrupt
> context.
> 
> For compatibillity, the wake up mechanism can't be symmetric, so we need
> in_stop_mode hack.
> 
> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> Reported-by: Sean Nyekjaer <sean@geanix.com>
> Tested-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> 
> Changelog:
> V1->V2:
> 	* add Reported-by tag.
> 	* rebase on patch: can:flexcan:fix stop mode acknowledgment.
> V2->V3:
> 	* rebase on linux-can/testing.
> 	* change into patch set.
> V3->V4:
> 	* add Tested-by tag.
> ---
>  drivers/net/can/flexcan.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> 1cd5179cb876..24cc386c4bce 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -286,6 +286,7 @@ struct flexcan_priv {
>  	const struct flexcan_devtype_data *devtype_data;
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
> +	bool in_stop_mode;
> 
>  	/* Read and Write APIs */
>  	u32 (*read)(void __iomem *addr);
> @@ -1670,6 +1671,8 @@ static int __maybe_unused flexcan_suspend(struct
> device *device)
>  			err = flexcan_enter_stop_mode(priv);
>  			if (err)
>  				return err;
> +
> +			priv->in_stop_mode = true;
>  		} else {
>  			err = flexcan_chip_disable(priv);
>  			if (err)
> @@ -1696,6 +1699,15 @@ static int __maybe_unused flexcan_resume(struct
> device *device)
>  		netif_device_attach(dev);
>  		netif_start_queue(dev);
>  		if (device_may_wakeup(device)) {
> +			if (priv->in_stop_mode) {
> +				flexcan_enable_wakeup_irq(priv, false);
> +				err = flexcan_exit_stop_mode(priv);
> +				if (err)
> +					return err;
> +
> +				priv->in_stop_mode = false;
> +			}
> +
>  			disable_irq_wake(dev->irq);
>  		} else {
>  			err = pm_runtime_force_resume(device); @@ -1732,6 +1744,11
> @@ static int __maybe_unused flexcan_noirq_suspend(struct device *device)
>  	struct net_device *dev = dev_get_drvdata(device);
>  	struct flexcan_priv *priv = netdev_priv(dev);
> 
> +	/* Need to enable wakeup interrupt in noirq suspend stage. Otherwise,
> +	 * it will trigger continuously wakeup interrupt if the wakeup event
> +	 * comes before noirq suspend stage, and simultaneously it has enter
> +	 * the stop mode.
> +	 */
>  	if (netif_running(dev) && device_may_wakeup(device))
>  		flexcan_enable_wakeup_irq(priv, true);
> 
> @@ -1744,11 +1761,17 @@ static int __maybe_unused
> flexcan_noirq_resume(struct device *device)
>  	struct flexcan_priv *priv = netdev_priv(dev);
>  	int err;
> 
> +	/* Need to exit stop mode in noirq resume stage. Otherwise, it will
> +	 * trigger continuously wakeup interrupt if the wakeup event comes,
> +	 * and simultaneously it has still in stop mode.
> +	 */
>  	if (netif_running(dev) && device_may_wakeup(device)) {
>  		flexcan_enable_wakeup_irq(priv, false);
>  		err = flexcan_exit_stop_mode(priv);
>  		if (err)
>  			return err;
> +
> +		priv->in_stop_mode = false;
>  	}
> 
>  	return 0;
> --
> 2.17.1