diff mbox series

can: rcar_can: Fix suspend/resume

Message ID 20210921051959.50309-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series can: rcar_can: Fix suspend/resume | expand

Checks

Context Check Description
netdev/tree_selection success Series ignored based on subject

Commit Message

Yoshihiro Shimoda Sept. 21, 2021, 5:19 a.m. UTC 
If the driver was not opened, rcar_can_suspend() should not call
clk_disable() because the clock was not enabled.

Fixes: fd1159318e55 ("can: add Renesas R-Car CAN driver")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Tested-by: Ayumi Nakamichi <ayumi.nakamichi.kf@renesas.com>
---
 drivers/net/can/rcar/rcar_can.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Ulrich Hecht Sept. 21, 2021, 10:38 a.m. UTC | #1 
> On 09/21/2021 7:19 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
>  
> If the driver was not opened, rcar_can_suspend() should not call
> clk_disable() because the clock was not enabled.
> 
> Fixes: fd1159318e55 ("can: add Renesas R-Car CAN driver")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Tested-by: Ayumi Nakamichi <ayumi.nakamichi.kf@renesas.com>
> ---
>  drivers/net/can/rcar/rcar_can.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> index 00e4533c8bdd..6b4eefb03044 100644
> --- a/drivers/net/can/rcar/rcar_can.c
> +++ b/drivers/net/can/rcar/rcar_can.c
> @@ -846,10 +846,12 @@ static int __maybe_unused rcar_can_suspend(struct device *dev)
>  	struct rcar_can_priv *priv = netdev_priv(ndev);
>  	u16 ctlr;
>  
> -	if (netif_running(ndev)) {
> -		netif_stop_queue(ndev);
> -		netif_device_detach(ndev);
> -	}
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	netif_stop_queue(ndev);
> +	netif_device_detach(ndev);
> +
>  	ctlr = readw(&priv->regs->ctlr);
>  	ctlr |= RCAR_CAN_CTLR_CANM_HALT;
>  	writew(ctlr, &priv->regs->ctlr);
> @@ -858,6 +860,7 @@ static int __maybe_unused rcar_can_suspend(struct device *dev)
>  	priv->can.state = CAN_STATE_SLEEPING;
>  
>  	clk_disable(priv->clk);
> +
>  	return 0;
>  }
>  
> @@ -868,6 +871,9 @@ static int __maybe_unused rcar_can_resume(struct device *dev)
>  	u16 ctlr;
>  	int err;
>  
> +	if (!netif_running(ndev))
> +		return 0;
> +
>  	err = clk_enable(priv->clk);
>  	if (err) {
>  		netdev_err(ndev, "clk_enable() failed, error %d\n", err);
> @@ -881,10 +887,9 @@ static int __maybe_unused rcar_can_resume(struct device *dev)
>  	writew(ctlr, &priv->regs->ctlr);
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
> -	if (netif_running(ndev)) {
> -		netif_device_attach(ndev);
> -		netif_start_queue(ndev);
> -	}
> +	netif_device_attach(ndev);
> +	netif_start_queue(ndev);
> +
>  	return 0;
>  }
>  
> -- 
> 2.25.1

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli
Simon Horman Sept. 22, 2021, 3:23 p.m. UTC | #2 
On Tue, Sep 21, 2021 at 12:38:13PM +0200, Ulrich Hecht wrote:
> 
> > On 09/21/2021 7:19 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> > 
> >  
> > If the driver was not opened, rcar_can_suspend() should not call
> > clk_disable() because the clock was not enabled.
> > 
> > Fixes: fd1159318e55 ("can: add Renesas R-Car CAN driver")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Tested-by: Ayumi Nakamichi <ayumi.nakamichi.kf@renesas.com>
> > ---
> >  drivers/net/can/rcar/rcar_can.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> > index 00e4533c8bdd..6b4eefb03044 100644
> > --- a/drivers/net/can/rcar/rcar_can.c
> > +++ b/drivers/net/can/rcar/rcar_can.c

...

> > @@ -858,6 +860,7 @@ static int __maybe_unused rcar_can_suspend(struct device *dev)
> >  	priv->can.state = CAN_STATE_SLEEPING;
> >  
> >  	clk_disable(priv->clk);
> > +
> >  	return 0;
> >  }
> >  

nit: this hunk seems unrelated to the rest of the patch

...
Biju Das Sept. 23, 2021, 1:30 p.m. UTC | #3 
Hi Shimoda-San,

Thanks for the patch. I have tested Wake on LAN(Magic packet) on Hihope RZ/G2M board
and this patches fixes the warning messages reported by kernel during suspend.

Tested-by: Biju Das <biju.das.jz@bp.renesas.com>


Before applying patch
------------------------
root@hihope-rzg2m:~# ethtool -s eth0 wol g
root@hihope-rzg2m:~# echo mem > /sys/power/state
[   92.220742] PM: suspend entry (s2idle)
[   92.228123] Filesystems sync: 0.003 seconds
[   92.248521] Freezing user space processes ... (elapsed 0.007 seconds) done.
[   92.263935] OOM killer disabled.
[   92.267265] Freezing remaining freezable tasks ... (elapsed 0.004 seconds) done.
[   92.279787] printk: Suspending console(s) (use no_console_suspend to debug)
[   92.310223] wl1271_sdio mmc2:0001:2: no wilink module was probed
[   92.541847] ------------[ cut here ]------------
[   92.541885] can-if1 already disabled
[   92.541932] WARNING: CPU: 0 PID: 480 at drivers/clk/clk.c:952 clk_core_disable+0x250/0x288
[   92.541962] CPU: 0 PID: 480 Comm: sh Not tainted 5.15.0-rc1-arm64-renesas-00402-gf0c16ee77cf1 #297
[   92.541973] Hardware name: HopeRun HiHope RZ/G2M with sub board (DT)
[   92.541981] pstate: 800000c5 (Nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   92.541990] pc : clk_core_disable+0x250/0x288
[   92.542000] lr : clk_core_disable+0x250/0x288
[   92.542008] sp : ffff80001263b980
[   92.542014] x29: ffff80001263b980 x28: ffff8000114ea508 x27: 0000000000000000
[   92.542037] x26: ffff8000113b0b48 x25: ffff80001115f000 x24: ffff8000114ea518
[   92.542057] x23: ffff0005c0b5f010 x22: ffff8000113b0b48 x21: 0000000000000000
[   92.542077] x20: ffff0005c0d09800 x19: ffff0005c0d09800 x18: 000000000000b948
[   92.542096] x17: 0000000000000000 x16: 000000000000b94e x15: ffff8000115c5e68
[   92.542116] x14: fffffffffffc63d7 x13: 0000000000000003 x12: ffff8000113d3e58
[   92.542135] x11: 0000000000000001 x10: 00000000000000f0 x9 : ffff8000111619e4
[   92.542154] x8 : ffff0005c67a4380 x7 : 0000000000000002 x6 : 0000000000000000
[   92.542173] x5 : ffff00063f727bf0 x4 : 0000000000000000 x3 : 0000000000000027
[   92.542192] x2 : 0000000000000023 x1 : 2b864f21865f9400 x0 : 0000000000000000
[   92.542212] Call trace:
[   92.542218]  clk_core_disable+0x250/0x288
[   92.542227]  clk_core_disable_lock+0x20/0x38
[   92.542236]  clk_disable+0x1c/0x28
[   92.542244]  rcar_can_suspend+0x74/0xb0
[   92.542257]  dpm_run_callback+0x5c/0x2b8
[   92.542268]  __device_suspend+0x110/0x5d8
[   92.542276]  dpm_suspend+0x124/0x438
[   92.542284]  dpm_suspend_start+0x78/0x90
[   92.542292]  suspend_devices_and_enter+0x10c/0xbb0
[   92.542304]  pm_suspend+0x274/0x338
[   92.542312]  state_store+0x88/0x110
[   92.542321]  kobj_attr_store+0x14/0x28
[   92.542335]  sysfs_kf_write+0x48/0x70
[   92.542345]  kernfs_fop_write_iter+0x118/0x1a8
[   92.542352]  new_sync_write+0xe4/0x180
[   92.542363]  vfs_write+0x29c/0x468
[   92.542371]  ksys_write+0x68/0xf0
[   92.542379]  __arm64_sys_write+0x18/0x20
[   92.542387]  invoke_syscall+0x40/0xf8
[   92.542398]  el0_svc_common.constprop.0+0xf0/0x110
[   92.542407]  do_el0_svc+0x20/0x78
[   92.542414]  el0_svc+0x50/0xe0
[   92.542423]  el0t_64_sync_handler+0xa8/0xb0
[   92.542431]  el0t_64_sync+0x158/0x15c
[   92.542440] irq event stamp: 46832
[   92.542445] hardirqs last  enabled at (46831): [<ffff800010c960dc>] _raw_spin_unlock_irqrestore+0x8c/0x90
[   92.542459] hardirqs last disabled at (46832): [<ffff8000105b4964>] clk_enable_lock+0xcc/0x118
[   92.542469] softirqs last  enabled at (42392): [<ffff800010010464>] _stext+0x464/0x5d8
[   92.542477] softirqs last disabled at (42387): [<ffff80001008f480>] irq_exit+0x198/0x1b8
[   92.542490] ---[ end trace e2078d4610595648 ]---
[   92.542527] ------------[ cut here ]------------
[   92.542532] can-if0 already disabled
[   92.542556] WARNING: CPU: 0 PID: 480 at drivers/clk/clk.c:952 clk_core_disable+0x250/0x288
[   92.542569] CPU: 0 PID: 480 Comm: sh Tainted: G        W         5.15.0-rc1-arm64-renesas-00402-gf0c16ee77cf1 #297
[   92.542578] Hardware name: HopeRun HiHope RZ/G2M with sub board (DT)
[   92.542583] pstate: 800000c5 (Nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   92.542590] pc : clk_core_disable+0x250/0x288
[   92.542598] lr : clk_core_disable+0x250/0x288
[   92.542606] sp : ffff80001263b980
[   92.542611] x29: ffff80001263b980 x28: ffff8000114ea508 x27: 0000000000000000
[   92.542631] x26: ffff8000113b0b48 x25: ffff80001115f000 x24: ffff8000114ea518
[   92.542650] x23: ffff0005c0b5d810 x22: ffff8000113b0b48 x21: 0000000000000000
[   92.542669] x20: ffff0005c0d09900 x19: ffff0005c0d09900 x18: 000000000000b948
[   92.542688] x17: 0000000000000000 x16: 000000000000b94e x15: ffff8000115c5e68
[   92.542707] x14: fffffffffffc6ec7 x13: 0000000000000003 x12: ffff8000113d3e58
[   92.542726] x11: 0000000000000001 x10: 00000000000000f0 x9 : ffff8000111619e4
[   92.542745] x8 : ffff0005c67a4380 x7 : 0000000000000002 x6 : 0000000000000000
[   92.542763] x5 : ffff00063f727bf0 x4 : 0000000000000000 x3 : 0000000000000027
[   92.542782] x2 : 0000000000000023 x1 : 2b864f21865f9400 x0 : 0000000000000000
[   92.542800] Call trace:
[   92.542806]  clk_core_disable+0x250/0x288
[   92.542814]  clk_core_disable_lock+0x20/0x38
[   92.542822]  clk_disable+0x1c/0x28
[   92.542828]  rcar_can_suspend+0x74/0xb0
[   92.542837]  dpm_run_callback+0x5c/0x2b8
[   92.542845]  __device_suspend+0x110/0x5d8
[   92.542852]  dpm_suspend+0x124/0x438
[   92.542860]  dpm_suspend_start+0x78/0x90
[   92.542868]  suspend_devices_and_enter+0x10c/0xbb0
[   92.542876]  pm_suspend+0x274/0x338
[   92.542884]  state_store+0x88/0x110
[   92.542892]  kobj_attr_store+0x14/0x28
[   92.542901]  sysfs_kf_write+0x48/0x70
[   92.542908]  kernfs_fop_write_iter+0x118/0x1a8
[   92.542915]  new_sync_write+0xe4/0x180
[   92.542923]  vfs_write+0x29c/0x468
[   92.542930]  ksys_write+0x68/0xf0
[   92.542938]  __arm64_sys_write+0x18/0x20
[   92.542945]  invoke_syscall+0x40/0xf8
[   92.542954]  el0_svc_common.constprop.0+0xf0/0x110
[   92.542962]  do_el0_svc+0x20/0x78
[   92.542969]  el0_svc+0x50/0xe0
[   92.542977]  el0t_64_sync_handler+0xa8/0xb0
[   92.542984]  el0t_64_sync+0x158/0x15c
[   92.542991] irq event stamp: 46852
[   92.542995] hardirqs last  enabled at (46851): [<ffff800010c960dc>] _raw_spin_unlock_irqrestore+0x8c/0x90
[   92.543004] hardirqs last disabled at (46852): [<ffff8000105b4964>] clk_enable_lock+0xcc/0x118
[   92.543013] softirqs last  enabled at (42392): [<ffff800010010464>] _stext+0x464/0x5d8
[   92.543020] softirqs last disabled at (42387): [<ffff80001008f480>] irq_exit+0x198/0x1b8
[   92.543029] ---[ end trace e2078d4610595649 ]---
[  148.223753] ravb e6800000.ethernet eth0: Link is Down
[  148.242725] RTL8211E Gigabit Ethernet e6800000.ethernet-ffffffff:00: attached PHY driver (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=199)
[  148.809790] OOM killer enabled.
[  148.813013] Restarting tasks ... done.
[  148.824071] PM: suspend exit
[  152.628415] ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
root@hihope-rzg2m:~#

After applying the patch:-
-------------------------

root@hihope-rzg2m:~# ethtool -s eth0 wol g
root@hihope-rzg2m:~# echo mem > /sys/power/state

[   38.607753] PM: suspend entry (s2idle)
[   38.616233] Filesystems sync: 0.004 seconds
[   38.638404] Freezing user space processes ... (elapsed 0.007 seconds) done.
[   38.654476] OOM killer disabled.
[   38.657879] Freezing remaining freezable tasks ... (elapsed 0.004 seconds) done.
[   38.670526] printk: Suspending console(s) (use no_console_suspend to debug)
[   38.698823] wl1271_sdio mmc2:0001:2: no wilink module was probed
[   45.828031] ravb e6800000.ethernet eth0: Link is Down
[   45.846544] RTL8211E Gigabit Ethernet e6800000.ethernet-ffffffff:00: attached PHY driver (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=199)
[   45.888434] OOM killer enabled.
[   45.891614] Restarting tasks ... done.
[   45.901692] PM: suspend exit
root@hihope-rzg2m:~#
root@hihope-rzg2m:~# [   49.468617] ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off

regards,
Biju


 

> -----Original Message-----
> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Sent: 21 September 2021 06:20
> To: wg@grandegger.com; mkl@pengutronix.de
> Cc: davem@davemloft.net; kuba@kernel.org; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Yoshihiro
> Shimoda <yoshihiro.shimoda.uh@renesas.com>; Ayumi Nakamichi
> <ayumi.nakamichi.kf@renesas.com>
> Subject: [PATCH] can: rcar_can: Fix suspend/resume
> 
> If the driver was not opened, rcar_can_suspend() should not call
> clk_disable() because the clock was not enabled.
> 
> Fixes: fd1159318e55 ("can: add Renesas R-Car CAN driver")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Tested-by: Ayumi Nakamichi <ayumi.nakamichi.kf@renesas.com>
> ---
>  drivers/net/can/rcar/rcar_can.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_can.c
> b/drivers/net/can/rcar/rcar_can.c index 00e4533c8bdd..6b4eefb03044 100644
> --- a/drivers/net/can/rcar/rcar_can.c
> +++ b/drivers/net/can/rcar/rcar_can.c
> @@ -846,10 +846,12 @@ static int __maybe_unused rcar_can_suspend(struct
> device *dev)
>  	struct rcar_can_priv *priv = netdev_priv(ndev);
>  	u16 ctlr;
> 
> -	if (netif_running(ndev)) {
> -		netif_stop_queue(ndev);
> -		netif_device_detach(ndev);
> -	}
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	netif_stop_queue(ndev);
> +	netif_device_detach(ndev);
> +
>  	ctlr = readw(&priv->regs->ctlr);
>  	ctlr |= RCAR_CAN_CTLR_CANM_HALT;
>  	writew(ctlr, &priv->regs->ctlr);
> @@ -858,6 +860,7 @@ static int __maybe_unused rcar_can_suspend(struct
> device *dev)
>  	priv->can.state = CAN_STATE_SLEEPING;
> 
>  	clk_disable(priv->clk);
> +
>  	return 0;
>  }
> 
> @@ -868,6 +871,9 @@ static int __maybe_unused rcar_can_resume(struct
> device *dev)
>  	u16 ctlr;
>  	int err;
> 
> +	if (!netif_running(ndev))
> +		return 0;
> +
>  	err = clk_enable(priv->clk);
>  	if (err) {
>  		netdev_err(ndev, "clk_enable() failed, error %d\n", err); @@ -
> 881,10 +887,9 @@ static int __maybe_unused rcar_can_resume(struct device
> *dev)
>  	writew(ctlr, &priv->regs->ctlr);
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> 
> -	if (netif_running(ndev)) {
> -		netif_device_attach(ndev);
> -		netif_start_queue(ndev);
> -	}
> +	netif_device_attach(ndev);
> +	netif_start_queue(ndev);
> +
>  	return 0;
>  }
> 
> --
> 2.25.1
Yoshihiro Shimoda Sept. 24, 2021, 7:43 a.m. UTC | #4 
Hi Simon-san,

> From: Simon Horman, Sent: Thursday, September 23, 2021 12:24 AM
> 
> On Tue, Sep 21, 2021 at 12:38:13PM +0200, Ulrich Hecht wrote:
> >
> > > On 09/21/2021 7:19 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> > >
> > >
> > > If the driver was not opened, rcar_can_suspend() should not call
> > > clk_disable() because the clock was not enabled.
> > >
> > > Fixes: fd1159318e55 ("can: add Renesas R-Car CAN driver")
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Tested-by: Ayumi Nakamichi <ayumi.nakamichi.kf@renesas.com>
> > > ---
> > >  drivers/net/can/rcar/rcar_can.c | 21 +++++++++++++--------
> > >  1 file changed, 13 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> > > index 00e4533c8bdd..6b4eefb03044 100644
> > > --- a/drivers/net/can/rcar/rcar_can.c
> > > +++ b/drivers/net/can/rcar/rcar_can.c
> 
> ...
> 
> > > @@ -858,6 +860,7 @@ static int __maybe_unused rcar_can_suspend(struct device *dev)
> > >  	priv->can.state = CAN_STATE_SLEEPING;
> > >
> > >  	clk_disable(priv->clk);
> > > +
> > >  	return 0;
> > >  }
> > >
> 
> nit: this hunk seems unrelated to the rest of the patch

Thank you for your comment! I'll remove this.

Best regards,
Yoshihiro Shimda

> ...
diff mbox series

Patch

diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index 00e4533c8bdd..6b4eefb03044 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -846,10 +846,12 @@  static int __maybe_unused rcar_can_suspend(struct device *dev)
 	struct rcar_can_priv *priv = netdev_priv(ndev);
 	u16 ctlr;
 
-	if (netif_running(ndev)) {
-		netif_stop_queue(ndev);
-		netif_device_detach(ndev);
-	}
+	if (!netif_running(ndev))
+		return 0;
+
+	netif_stop_queue(ndev);
+	netif_device_detach(ndev);
+
 	ctlr = readw(&priv->regs->ctlr);
 	ctlr |= RCAR_CAN_CTLR_CANM_HALT;
 	writew(ctlr, &priv->regs->ctlr);
@@ -858,6 +860,7 @@  static int __maybe_unused rcar_can_suspend(struct device *dev)
 	priv->can.state = CAN_STATE_SLEEPING;
 
 	clk_disable(priv->clk);
+
 	return 0;
 }
 
@@ -868,6 +871,9 @@  static int __maybe_unused rcar_can_resume(struct device *dev)
 	u16 ctlr;
 	int err;
 
+	if (!netif_running(ndev))
+		return 0;
+
 	err = clk_enable(priv->clk);
 	if (err) {
 		netdev_err(ndev, "clk_enable() failed, error %d\n", err);
@@ -881,10 +887,9 @@  static int __maybe_unused rcar_can_resume(struct device *dev)
 	writew(ctlr, &priv->regs->ctlr);
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	if (netif_running(ndev)) {
-		netif_device_attach(ndev);
-		netif_start_queue(ndev);
-	}
+	netif_device_attach(ndev);
+	netif_start_queue(ndev);
+
 	return 0;
 }