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

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

[Joakim Zhang]

From: Sean Nyekjaer <sean@geanix.com>

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.

This patch can fix deadlock when using self wakeup, it happenes to be
able to fix another issue that frames out-of-order in first IRQ handler
run after wakeup.

In wakeup case, after system resume, frames received out-of-order,the
problem is wakeup latency from frame reception to IRQ handler is much
bigger than the counter overflow. This means it's impossible to sort the
CAN frames by timestamp. The reason is that controller exits stop mode
during noirq resume, then it can receive the frame immediately. If
noirq reusme stage consumes much time, it will extend interrupt response
time.

Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index a929cdda9ab2..43fd187768f1 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -134,8 +134,7 @@
 	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)
 #define FLEXCAN_ESR_ALL_INT \
 	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
-	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
-	 FLEXCAN_ESR_WAK_INT)
+	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
 
 /* FLEXCAN interrupt flag register (IFLAG) bits */
 /* Errata ERR005829 step7: Reserve first valid MB */
@@ -960,6 +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 
 	reg_esr = priv->read(&regs->esr);
 
+	/* ACK wakeup interrupt */
+	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
+		handled = IRQ_HANDLED;
+		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
+	}
+
 	/* ACK all bus error and state change IRQ sources */
 	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
 		handled = IRQ_HANDLED;
@@ -1722,6 +1727,9 @@ static int __maybe_unused flexcan_resume(struct device *device)
 		netif_start_queue(dev);
 		if (device_may_wakeup(device)) {
 			disable_irq_wake(dev->irq);
+			err = flexcan_exit_stop_mode(priv);
+			if (err)
+				return err;
 		} else {
 			err = pm_runtime_force_resume(device);
 			if (err)
@@ -1767,14 +1775,9 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct flexcan_priv *priv = netdev_priv(dev);
-	int err;
 
-	if (netif_running(dev) && device_may_wakeup(device)) {
+	if (netif_running(dev) && device_may_wakeup(device))
 		flexcan_enable_wakeup_irq(priv, false);
-		err = flexcan_exit_stop_mode(priv);
-		if (err)
-			return err;
-	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH 2/3] can: flexcan: change the way of stop mode acknowledgment

[Joakim Zhang]

Stop mode is entered when Stop mode is requested at chip level and
MCR[LPM_ACK] is asserted by the FlexCAN.

Double check with IP owner, should poll MCR[LPM_ACK] for stop mode
acknowledgment, not the acknowledgment from chip level.

Fixes: 5f186c257fa4(can: flexcan: fix stop mode acknowledgment)
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 64 ++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 43fd187768f1..dd91d8d6b5a6 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -388,6 +388,34 @@ static struct flexcan_mb __iomem *flexcan_get_mb(const struct flexcan_priv *priv
 		(&priv->regs->mb[bank][priv->mb_size * mb_index]);
 }
 
+static int flexcan_enter_low_power_ack(struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
+
+	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
+		udelay(10);
+
+	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int flexcan_exit_low_power_ack(struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
+
+	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
+		udelay(10);
+
+	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
@@ -406,7 +434,6 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
 static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int ackval;
 	u32 reg_mcr;
 
 	reg_mcr = priv->read(&regs->mcr);
@@ -418,35 +445,24 @@ static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
 			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
 
 	/* get stop acknowledgment */
-	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
-				     ackval, ackval & (1 << priv->stm.ack_bit),
-				     0, FLEXCAN_TIMEOUT_US))
-		return -ETIMEDOUT;
-
-	return 0;
+	return flexcan_enter_low_power_ack(priv);
 }
 
 static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int ackval;
 	u32 reg_mcr;
 
 	/* remove stop request */
 	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
 			   1 << priv->stm.req_bit, 0);
 
-	/* get stop acknowledgment */
-	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
-				     ackval, !(ackval & (1 << priv->stm.ack_bit)),
-				     0, FLEXCAN_TIMEOUT_US))
-		return -ETIMEDOUT;
-
 	reg_mcr = priv->read(&regs->mcr);
 	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
 	priv->write(reg_mcr, &regs->mcr);
 
-	return 0;
+	/* get stop acknowledgment */
+	return flexcan_exit_low_power_ack(priv);
 }
 
 static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
@@ -505,39 +521,25 @@ static inline int flexcan_transceiver_disable(const struct flexcan_priv *priv)
 static int flexcan_chip_enable(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
 	u32 reg;
 
 	reg = priv->read(&regs->mcr);
 	reg &= ~FLEXCAN_MCR_MDIS;
 	priv->write(reg, &regs->mcr);
 
-	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
-		udelay(10);
-
-	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
-		return -ETIMEDOUT;
-
-	return 0;
+	return flexcan_exit_low_power_ack(priv);
 }
 
 static int flexcan_chip_disable(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
-	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
 	u32 reg;
 
 	reg = priv->read(&regs->mcr);
 	reg |= FLEXCAN_MCR_MDIS;
 	priv->write(reg, &regs->mcr);
 
-	while (timeout-- && !(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
-		udelay(10);
-
-	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
-		return -ETIMEDOUT;
-
-	return 0;
+	return flexcan_enter_low_power_ack(priv);
 }
 
 static int flexcan_chip_freeze(struct flexcan_priv *priv)
-- 
2.17.1

[PATCH 3/3] can: flexcan: add LPSR mode support

[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>
---
 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 dd91d8d6b5a6..f75d8099a035 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"
@@ -1691,7 +1692,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
@@ -1703,25 +1704,27 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 			if (err)
 				return err;
 		} 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)) {
@@ -1733,15 +1736,19 @@ static int __maybe_unused flexcan_resume(struct device *device)
 			if (err)
 				return err;
 		} 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 1/3] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang]

Hi Sean,

I remember that you are the first one sending out the patch to fix this issue, and I NACK the patch before.
I am so sorry for that, it can work fine after testing at my side. Could you help double check at your side for
this patch? Both wakeup from totally suspend and wakeup from suspending?

With this patch, we can fix two problems:
1) fix deadlock when using self wakeup
2) frames out-of-order in first IRQ handler run after wakeup

Thanks a lot!

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2019年11月15日 13:03
> To: mkl@pengutronix.de; sean@geanix.com; linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org; Joakim Zhang
> <qiangqing.zhang@nxp.com>
> Subject: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
> 
> From: Sean Nyekjaer <sean@geanix.com>
> 
> 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.
> 
> This patch can fix deadlock when using self wakeup, it happenes to be able to
> fix another issue that frames out-of-order in first IRQ handler run after wakeup.
> 
> In wakeup case, after system resume, frames received out-of-order,the
> problem is wakeup latency from frame reception to IRQ handler is much bigger
> than the counter overflow. This means it's impossible to sort the CAN frames
> by timestamp. The reason is that controller exits stop mode during noirq
> resume, then it can receive the frame immediately. If noirq reusme stage
> consumes much time, it will extend interrupt response time.
> 
> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> a929cdda9ab2..43fd187768f1 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -134,8 +134,7 @@
>  	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)  #define
> FLEXCAN_ESR_ALL_INT \
>  	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> -	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
> -	 FLEXCAN_ESR_WAK_INT)
> +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> 
>  /* FLEXCAN interrupt flag register (IFLAG) bits */
>  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -960,6 +959,12
> @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> 
>  	reg_esr = priv->read(&regs->esr);
> 
> +	/* ACK wakeup interrupt */
> +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
> +		handled = IRQ_HANDLED;
> +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
> +	}
> +
>  	/* ACK all bus error and state change IRQ sources */
>  	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>  		handled = IRQ_HANDLED;
> @@ -1722,6 +1727,9 @@ static int __maybe_unused flexcan_resume(struct
> device *device)
>  		netif_start_queue(dev);
>  		if (device_may_wakeup(device)) {
>  			disable_irq_wake(dev->irq);
> +			err = flexcan_exit_stop_mode(priv);
> +			if (err)
> +				return err;
>  		} else {
>  			err = pm_runtime_force_resume(device);
>  			if (err)
> @@ -1767,14 +1775,9 @@ static int __maybe_unused
> flexcan_noirq_resume(struct device *device)  {
>  	struct net_device *dev = dev_get_drvdata(device);
>  	struct flexcan_priv *priv = netdev_priv(dev);
> -	int err;
> 
> -	if (netif_running(dev) && device_may_wakeup(device)) {
> +	if (netif_running(dev) && device_may_wakeup(device))
>  		flexcan_enable_wakeup_irq(priv, false);
> -		err = flexcan_exit_stop_mode(priv);
> -		if (err)
> -			return err;
> -	}
> 
>  	return 0;
>  }
> --
> 2.17.1

Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup

[Sean Nyekjaer]

On 15/11/2019 06.09, Joakim Zhang wrote:
> 
> Hi Sean,
> 
> I remember that you are the first one sending out the patch to fix this issue, and I NACK the patch before.
> I am so sorry for that, it can work fine after testing at my side. Could you help double check at your side for
> this patch? Both wakeup from totally suspend and wakeup from suspending?
> 
> With this patch, we can fix two problems:
> 1) fix deadlock when using self wakeup
> 2) frames out-of-order in first IRQ handler run after wakeup
> 
> Thanks a lot!
> 
> Best Regards,
> Joakim Zhang

Hi Joakim,



We are mostly listening for broadcast packages, so we haven't noticed 
frames out-of-order :-)



I have checked this series, it comes out of suspend every time :-)

/Sean

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

[Joakim Zhang]

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: 2019年11月15日 16:54
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de;
> linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
> 
> 
> 
> On 15/11/2019 06.09, Joakim Zhang wrote:
> >
> > Hi Sean,
> >
> > I remember that you are the first one sending out the patch to fix this issue,
> and I NACK the patch before.
> > I am so sorry for that, it can work fine after testing at my side.
> > Could you help double check at your side for this patch? Both wakeup from
> totally suspend and wakeup from suspending?
> >
> > With this patch, we can fix two problems:
> > 1) fix deadlock when using self wakeup
> > 2) frames out-of-order in first IRQ handler run after wakeup
> >
> > Thanks a lot!
> >
> > Best Regards,
> > Joakim Zhang
> 
> Hi Joakim,
> 
> 
> 
> We are mostly listening for broadcast packages, so we haven't noticed frames
> out-of-order :-)

Okay, I have discussed with Marc before, this could be possible. You can test with two boards, one receive and another transmit.
> 
> 
> I have checked this series, it comes out of suspend every time :-)

I am not quite understand, could you explain more, this patch cannot fix deadlock issue?

Best Regards,
Joakim Zhang
> /Sean

Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup

[Sean Nyekjaer]

On 15/11/2019 10.06, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Sean Nyekjaer <sean@geanix.com>
>> Sent: 2019年11月15日 16:54
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de;
>> linux-can@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
>> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
>>
>>
>>
>> On 15/11/2019 06.09, Joakim Zhang wrote:
>>>
>>> Hi Sean,
>>>
>>> I remember that you are the first one sending out the patch to fix this issue,
>> and I NACK the patch before.
>>> I am so sorry for that, it can work fine after testing at my side.
>>> Could you help double check at your side for this patch? Both wakeup from
>> totally suspend and wakeup from suspending?
>>>
>>> With this patch, we can fix two problems:
>>> 1) fix deadlock when using self wakeup
>>> 2) frames out-of-order in first IRQ handler run after wakeup
>>>
>>> Thanks a lot!
>>>
>>> Best Regards,
>>> Joakim Zhang
>>
>> Hi Joakim,
>>
>>
>>
>> We are mostly listening for broadcast packages, so we haven't noticed frames
>> out-of-order :-)
> 
> Okay, I have discussed with Marc before, this could be possible. You can test with two boards, one receive and another transmit.
>>
>>
>> I have checked this series, it comes out of suspend every time :-)
> 
> I am not quite understand, could you explain more, this patch cannot fix deadlock issue?

It fixes the deadlock issue :)
> 
> Best Regards,
> Joakim Zhang
>> /Sean
> 

Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup

[Sean Nyekjaer]

On 15/11/2019 06.03, Joakim Zhang wrote:
> From: Sean Nyekjaer <sean@geanix.com>
> 
> 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.
> 
> This patch can fix deadlock when using self wakeup, it happenes to be
> able to fix another issue that frames out-of-order in first IRQ handler
> run after wakeup.
> 
> In wakeup case, after system resume, frames received out-of-order,the
> problem is wakeup latency from frame reception to IRQ handler is much
> bigger than the counter overflow. This means it's impossible to sort the
> CAN frames by timestamp. The reason is that controller exits stop mode
> during noirq resume, then it can receive the frame immediately. If
> noirq reusme stage consumes much time, it will extend interrupt response
> time.
> 
> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Hi Joakim and Marc

We have quite a few devices in the field where flexcan is stuck in 
Stop-Mode. We do not have the possibility to cold reboot them, and hot 
reboot will not get flexcan out of stop-mode.
So flexcan comes up with:
[  279.444077] flexcan: probe of 2090000.flexcan failed with error -110
[  279.501405] flexcan: probe of 2094000.flexcan failed with error -110

They are on, de3578c198c6 ("can: flexcan: add self wakeup support")

Would it be a solution to add a check in the probe function to pull it 
out of stop-mode?

/Sean

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

[Joakim Zhang]

> -----Original Message-----
> From: linux-can-owner@vger.kernel.org <linux-can-owner@vger.kernel.org>
> On Behalf Of Sean Nyekjaer
> Sent: 2019年11月15日 17:08
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de
> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
> 
> 
> 
> On 15/11/2019 06.03, Joakim Zhang wrote:
> > From: Sean Nyekjaer <sean@geanix.com>
> >
> > 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.
> >
> > This patch can fix deadlock when using self wakeup, it happenes to be
> > able to fix another issue that frames out-of-order in first IRQ
> > handler run after wakeup.
> >
> > In wakeup case, after system resume, frames received out-of-order,the
> > problem is wakeup latency from frame reception to IRQ handler is much
> > bigger than the counter overflow. This means it's impossible to sort
> > the CAN frames by timestamp. The reason is that controller exits stop
> > mode during noirq resume, then it can receive the frame immediately.
> > If noirq reusme stage consumes much time, it will extend interrupt
> > response time.
> >
> > Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> 
> Hi Joakim and Marc
> 
> We have quite a few devices in the field where flexcan is stuck in Stop-Mode.
> We do not have the possibility to cold reboot them, and hot reboot will not get
> flexcan out of stop-mode.
> So flexcan comes up with:
> [  279.444077] flexcan: probe of 2090000.flexcan failed with error -110
> [  279.501405] flexcan: probe of 2094000.flexcan failed with error -110
> 
> They are on, de3578c198c6 ("can: flexcan: add self wakeup support")
> 
> Would it be a solution to add a check in the probe function to pull it out of
> stop-mode?

Hi Sean,

I am not sure, I will try to implement it.

Best Regards,
Joakim Zhang
> /Sean

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

[Joakim Zhang]

> -----Original Message-----
> From: linux-can-owner@vger.kernel.org <linux-can-owner@vger.kernel.org>
> On Behalf Of Sean Nyekjaer
> Sent: 2019年11月15日 17:08
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de
> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
> 
> 
> 
> On 15/11/2019 06.03, Joakim Zhang wrote:
> > From: Sean Nyekjaer <sean@geanix.com>
> >
> > 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.
> >
> > This patch can fix deadlock when using self wakeup, it happenes to be
> > able to fix another issue that frames out-of-order in first IRQ
> > handler run after wakeup.
> >
> > In wakeup case, after system resume, frames received out-of-order,the
> > problem is wakeup latency from frame reception to IRQ handler is much
> > bigger than the counter overflow. This means it's impossible to sort
> > the CAN frames by timestamp. The reason is that controller exits stop
> > mode during noirq resume, then it can receive the frame immediately.
> > If noirq reusme stage consumes much time, it will extend interrupt
> > response time.
> >
> > Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> 
> Hi Joakim and Marc
> 
> We have quite a few devices in the field where flexcan is stuck in Stop-Mode.
> We do not have the possibility to cold reboot them, and hot reboot will not get
> flexcan out of stop-mode.
> So flexcan comes up with:
> [  279.444077] flexcan: probe of 2090000.flexcan failed with error -110
> [  279.501405] flexcan: probe of 2094000.flexcan failed with error -110
> 
> They are on, de3578c198c6 ("can: flexcan: add self wakeup support")
> 
> Would it be a solution to add a check in the probe function to pull it out of
> stop-mode?

Hi Sean,

Soft reset cannot be applied when clocks are shut down in a low power mode. The module should be first removed from low power mode, and then soft reset can be applied.
And exit from stop mode happens when the Stop mode request is removed, or when activity is detected on the CAN bus and the Self Wake Up mechanism is enabled.

So from my point of view, we can add a check in the probe function to pull it out of stop mode, since controller actually could be stuck in stop mode if suspend/resume failed and users just 
want a warm reset for the system.

Could you please tell me how can I generate a warm reset? AFAIK, both "reboot" command put into prompt and RST KEY in our EVK board all play a role of cold reset.

Best Regards,
Joakim Zhang
> /Sean

Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup

[Sean Nyekjaer]

On 18/11/2019 08.05, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: linux-can-owner@vger.kernel.org <linux-can-owner@vger.kernel.org>
>> On Behalf Of Sean Nyekjaer
>> Sent: 2019年11月15日 17:08
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de
>> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
>> netdev@vger.kernel.org
>> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
>>
>>
>>
>> On 15/11/2019 06.03, Joakim Zhang wrote:
>>> From: Sean Nyekjaer <sean@geanix.com>
>>>
>>> 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.
>>>
>>> This patch can fix deadlock when using self wakeup, it happenes to be
>>> able to fix another issue that frames out-of-order in first IRQ
>>> handler run after wakeup.
>>>
>>> In wakeup case, after system resume, frames received out-of-order,the
>>> problem is wakeup latency from frame reception to IRQ handler is much
>>> bigger than the counter overflow. This means it's impossible to sort
>>> the CAN frames by timestamp. The reason is that controller exits stop
>>> mode during noirq resume, then it can receive the frame immediately.
>>> If noirq reusme stage consumes much time, it will extend interrupt
>>> response time.
>>>
>>> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>>
>> Hi Joakim and Marc
>>
>> We have quite a few devices in the field where flexcan is stuck in Stop-Mode.
>> We do not have the possibility to cold reboot them, and hot reboot will not get
>> flexcan out of stop-mode.
>> So flexcan comes up with:
>> [  279.444077] flexcan: probe of 2090000.flexcan failed with error -110
>> [  279.501405] flexcan: probe of 2094000.flexcan failed with error -110
>>
>> They are on, de3578c198c6 ("can: flexcan: add self wakeup support")
>>
>> Would it be a solution to add a check in the probe function to pull it out of
>> stop-mode?
> 
> Hi Sean,
> 
> Soft reset cannot be applied when clocks are shut down in a low power mode. The module should be first removed from low power mode, and then soft reset can be applied.
> And exit from stop mode happens when the Stop mode request is removed, or when activity is detected on the CAN bus and the Self Wake Up mechanism is enabled.
> 
> So from my point of view, we can add a check in the probe function to pull it out of stop mode, since controller actually could be stuck in stop mode if suspend/resume failed and users just
> want a warm reset for the system.

Exactly what I thought could be done :)

> 
> Could you please tell me how can I generate a warm reset? AFAIK, both "reboot" command put into prompt and RST KEY in our EVK board all play a role of cold reset.

Warm reset is just `reboot` :-) Cold is poweroff...

/Sean

> 
> Best Regards,
> Joakim Zhang
>> /Sean

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

[Joakim Zhang]

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: 2019年11月18日 15:53
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de
> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
> 
> 
> 
> On 18/11/2019 08.05, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: linux-can-owner@vger.kernel.org
> >> <linux-can-owner@vger.kernel.org> On Behalf Of Sean Nyekjaer
> >> Sent: 2019年11月15日 17:08
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de
> >> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> >> netdev@vger.kernel.org
> >> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self
> >> wakeup
> >>
> >>
> >>
> >> On 15/11/2019 06.03, Joakim Zhang wrote:
> >>> From: Sean Nyekjaer <sean@geanix.com>
> >>>
> >>> 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.
> >>>
> >>> This patch can fix deadlock when using self wakeup, it happenes to
> >>> be able to fix another issue that frames out-of-order in first IRQ
> >>> handler run after wakeup.
> >>>
> >>> In wakeup case, after system resume, frames received
> >>> out-of-order,the problem is wakeup latency from frame reception to
> >>> IRQ handler is much bigger than the counter overflow. This means
> >>> it's impossible to sort the CAN frames by timestamp. The reason is
> >>> that controller exits stop mode during noirq resume, then it can receive the
> frame immediately.
> >>> If noirq reusme stage consumes much time, it will extend interrupt
> >>> response time.
> >>>
> >>> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> >>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> >>
> >> Hi Joakim and Marc
> >>
> >> We have quite a few devices in the field where flexcan is stuck in Stop-Mode.
> >> We do not have the possibility to cold reboot them, and hot reboot
> >> will not get flexcan out of stop-mode.
> >> So flexcan comes up with:
> >> [  279.444077] flexcan: probe of 2090000.flexcan failed with error
> >> -110 [  279.501405] flexcan: probe of 2094000.flexcan failed with
> >> error -110
> >>
> >> They are on, de3578c198c6 ("can: flexcan: add self wakeup support")
> >>
> >> Would it be a solution to add a check in the probe function to pull
> >> it out of stop-mode?
> >
> > Hi Sean,
> >
> > Soft reset cannot be applied when clocks are shut down in a low power mode.
> The module should be first removed from low power mode, and then soft reset
> can be applied.
> > And exit from stop mode happens when the Stop mode request is removed,
> or when activity is detected on the CAN bus and the Self Wake Up mechanism is
> enabled.
> >
> > So from my point of view, we can add a check in the probe function to
> > pull it out of stop mode, since controller actually could be stuck in stop mode
> if suspend/resume failed and users just want a warm reset for the system.
> 
> Exactly what I thought could be done :)
> 
> >
> > Could you please tell me how can I generate a warm reset? AFAIK, both
> "reboot" command put into prompt and RST KEY in our EVK board all play a role
> of cold reset.
> 
> Warm reset is just `reboot` :-) Cold is poweroff...

I add the code flexcan_enter_stop_mode(priv) at the end of the probe function, 'reboot' the system directly after system active.
However, I do not meet the probe error, it can probe successfully. Do you know the reason?

Best Regards,
Joakim Zhang
> /Sean
> 
> >
> > Best Regards,
> > Joakim Zhang
> >> /Sean

Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup

[Sean Nyekjaer]

On 18/11/2019 09.04, Joakim Zhang wrote:
>>>> Hi Joakim and Marc
>>>>
>>>> We have quite a few devices in the field where flexcan is stuck in Stop-Mode.
>>>> We do not have the possibility to cold reboot them, and hot reboot
>>>> will not get flexcan out of stop-mode.
>>>> So flexcan comes up with:
>>>> [  279.444077] flexcan: probe of 2090000.flexcan failed with error
>>>> -110 [  279.501405] flexcan: probe of 2094000.flexcan failed with
>>>> error -110
>>>>
>>>> They are on, de3578c198c6 ("can: flexcan: add self wakeup support")
>>>>
>>>> Would it be a solution to add a check in the probe function to pull
>>>> it out of stop-mode?
>>>
>>> Hi Sean,
>>>
>>> Soft reset cannot be applied when clocks are shut down in a low power mode.
>> The module should be first removed from low power mode, and then soft reset
>> can be applied.
>>> And exit from stop mode happens when the Stop mode request is removed,
>> or when activity is detected on the CAN bus and the Self Wake Up mechanism is
>> enabled.
>>>
>>> So from my point of view, we can add a check in the probe function to
>>> pull it out of stop mode, since controller actually could be stuck in stop mode
>> if suspend/resume failed and users just want a warm reset for the system.
>>
>> Exactly what I thought could be done :)
>>
>>>
>>> Could you please tell me how can I generate a warm reset? AFAIK, both
>> "reboot" command put into prompt and RST KEY in our EVK board all play a role
>> of cold reset.
>>
>> Warm reset is just `reboot` :-) Cold is poweroff...
> 
> I add the code flexcan_enter_stop_mode(priv) at the end of the probe function, 'reboot' the system directly after system active.
> However, I do not meet the probe error, it can probe successfully. Do you know the reason?

You will have to get it in the deadlock situation first :)

This can be achieved by using de3578c198c6 and sending can messages to 
both can interfaces while calling suspend.

/Sean

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

[Joakim Zhang]

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: 2019年11月18日 16:22
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; mkl@pengutronix.de
> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
> 
> 
> 
> On 18/11/2019 09.04, Joakim Zhang wrote:
> >>>> Hi Joakim and Marc
> >>>>
> >>>> We have quite a few devices in the field where flexcan is stuck in
> Stop-Mode.
> >>>> We do not have the possibility to cold reboot them, and hot reboot
> >>>> will not get flexcan out of stop-mode.
> >>>> So flexcan comes up with:
> >>>> [  279.444077] flexcan: probe of 2090000.flexcan failed with error
> >>>> -110 [  279.501405] flexcan: probe of 2094000.flexcan failed with
> >>>> error -110
> >>>>
> >>>> They are on, de3578c198c6 ("can: flexcan: add self wakeup support")
> >>>>
> >>>> Would it be a solution to add a check in the probe function to pull
> >>>> it out of stop-mode?
> >>>
> >>> Hi Sean,
> >>>
> >>> Soft reset cannot be applied when clocks are shut down in a low power
> mode.
> >> The module should be first removed from low power mode, and then soft
> >> reset can be applied.
> >>> And exit from stop mode happens when the Stop mode request is
> >>> removed,
> >> or when activity is detected on the CAN bus and the Self Wake Up
> >> mechanism is enabled.
> >>>
> >>> So from my point of view, we can add a check in the probe function
> >>> to pull it out of stop mode, since controller actually could be
> >>> stuck in stop mode
> >> if suspend/resume failed and users just want a warm reset for the system.
> >>
> >> Exactly what I thought could be done :)
> >>
> >>>
> >>> Could you please tell me how can I generate a warm reset? AFAIK,
> >>> both
> >> "reboot" command put into prompt and RST KEY in our EVK board all
> >> play a role of cold reset.
> >>
> >> Warm reset is just `reboot` :-) Cold is poweroff...
> >
> > I add the code flexcan_enter_stop_mode(priv) at the end of the probe
> function, 'reboot' the system directly after system active.
> > However, I do not meet the probe error, it can probe successfully. Do you
> know the reason?
> 
> You will have to get it in the deadlock situation first :)

As you said, if we need get it into the deadlock situation first, then this patch has fixed it, that means the problem goes away?

With this patch, if CAN enter stop mode when suspend, it certainly can exit stop mode when resume. Unless such activity occurs, CAN enter
stop mode, however system hang before it exit stop mode. Right? 
So I do not know how to reproduce this issue after applying this patch. 

And the case I mentioned in last response, CAN enter stop mode at the end of probe function, then "reboot" the system should be the same situation
with " This can be achieved by using de3578c198c6 and sending can messages to both can interfaces while calling suspend." The aim is both that let controller
enter stop mode, then 'reboot' the system. Right?

Best Regards,
Joakim Zhang
> This can be achieved by using de3578c198c6 and sending can messages to both
> can interfaces while calling suspend.
> 
> /Sean