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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Nov 2019 05:09:53 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     "mkl@...gutronix.de" <mkl@...gutronix.de>,
        "sean@...nix.com" <sean@...nix.com>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>
CC:     dl-linux-imx <linux-imx@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup


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@....com>
> Sent: 2019年11月15日 13:03
> To: mkl@...gutronix.de; sean@...nix.com; linux-can@...r.kernel.org
> Cc: dl-linux-imx <linux-imx@....com>; netdev@...r.kernel.org; Joakim Zhang
> <qiangqing.zhang@....com>
> Subject: [PATCH 1/3] can: flexcan: fix deadlock when using self wakeup
> 
> From: Sean Nyekjaer <sean@...nix.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@...nix.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@....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