[PATCH net 0/2] Fix missing rtnl lock in suspend path

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net 0/2] Fix missing rtnl lock in suspend path
@ 2025-01-22 16:19 Kory Maincent
  2025-01-22 16:19 ` [PATCH net 1/2] net: ravb: " Kory Maincent
  2025-01-22 16:19 ` [PATCH net 2/2] net: sh_eth: " Kory Maincent
  0 siblings, 2 replies; 13+ messages in thread
From: Kory Maincent @ 2025-01-22 16:19 UTC (permalink / raw)
  To: Paul Barker, Niklas Söderlund, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov,
	Sergei Shtylyov
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel, Kory Maincent

Fix the suspend path by ensuring the rtnl lock is held where required.
Calls to open, close and WOL operations must be performed under the
rtnl lock to prevent conflicts with ongoing ndo operations.

Discussion about this issue can be found here:
https://lore.kernel.org/netdev/20250120141926.1290763-1-kory.maincent@bootlin.com/

While working on the ravb fix, it was discovered that the sh_eth driver
has the same issue. This patch series addresses both drivers.

I do not have access to hardware for either of these MACs, so it would
be great if maintainers or others with the relevant boards could test
these fixes.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Kory Maincent (2):
      net: ravb: Fix missing rtnl lock in suspend path
      net: sh_eth: Fix missing rtnl lock in suspend path

 drivers/net/ethernet/renesas/ravb_main.c | 19 +++++++++++++++----
 drivers/net/ethernet/renesas/sh_eth.c    |  4 ++++
 2 files changed, 19 insertions(+), 4 deletions(-)
---
base-commit: b1754a69e7be48a64b3cdb0df60a96d97959da73
change-id: 20250122-fix_missing_rtnl_lock_phy_disconnect-fc1d6f5db705

Best regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 [PATCH net 0/2] Fix missing rtnl lock in suspend path Kory Maincent
@ 2025-01-22 16:19 ` Kory Maincent
  2025-01-22 17:22   ` Niklas Söderlund
                     ` (3 more replies)
  2025-01-22 16:19 ` [PATCH net 2/2] net: sh_eth: " Kory Maincent
  1 sibling, 4 replies; 13+ messages in thread
From: Kory Maincent @ 2025-01-22 16:19 UTC (permalink / raw)
  To: Paul Barker, Niklas Söderlund, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov,
	Sergei Shtylyov
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel, Kory Maincent

Fix the suspend path by ensuring the rtnl lock is held where required.
Calls to ravb_open, ravb_close and wol operations must be performed under
the rtnl lock to prevent conflicts with ongoing ndo operations.

Without this fix, the following warning is triggered:
[   39.032969] =============================
[   39.032983] WARNING: suspicious RCU usage
[   39.033019] -----------------------------
[   39.033033] drivers/net/phy/phy_device.c:2004 suspicious
rcu_dereference_protected() usage!
...
[   39.033597] stack backtrace:
[   39.033613] CPU: 0 UID: 0 PID: 174 Comm: python3 Not tainted
6.13.0-rc7-next-20250116-arm64-renesas-00002-g35245dfdc62c #7
[   39.033623] Hardware name: Renesas SMARC EVK version 2 based on
r9a08g045s33 (DT)
[   39.033628] Call trace:
[   39.033633]  show_stack+0x14/0x1c (C)
[   39.033652]  dump_stack_lvl+0xb4/0xc4
[   39.033664]  dump_stack+0x14/0x1c
[   39.033671]  lockdep_rcu_suspicious+0x16c/0x22c
[   39.033682]  phy_detach+0x160/0x190
[   39.033694]  phy_disconnect+0x40/0x54
[   39.033703]  ravb_close+0x6c/0x1cc
[   39.033714]  ravb_suspend+0x48/0x120
[   39.033721]  dpm_run_callback+0x4c/0x14c
[   39.033731]  device_suspend+0x11c/0x4dc
[   39.033740]  dpm_suspend+0xdc/0x214
[   39.033748]  dpm_suspend_start+0x48/0x60
[   39.033758]  suspend_devices_and_enter+0x124/0x574
[   39.033769]  pm_suspend+0x1ac/0x274
[   39.033778]  state_store+0x88/0x124
[   39.033788]  kobj_attr_store+0x14/0x24
[   39.033798]  sysfs_kf_write+0x48/0x6c
[   39.033808]  kernfs_fop_write_iter+0x118/0x1a8
[   39.033817]  vfs_write+0x27c/0x378
[   39.033825]  ksys_write+0x64/0xf4
[   39.033833]  __arm64_sys_write+0x18/0x20
[   39.033841]  invoke_syscall+0x44/0x104
[   39.033852]  el0_svc_common.constprop.0+0xb4/0xd4
[   39.033862]  do_el0_svc+0x18/0x20
[   39.033870]  el0_svc+0x3c/0xf0
[   39.033880]  el0t_64_sync_handler+0xc0/0xc4
[   39.033888]  el0t_64_sync+0x154/0x158
[   39.041274] ravb 11c30000.ethernet eth0: Link is Down

Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index bc395294a32d..2c6d8e4966c3 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -3217,10 +3217,15 @@ static int ravb_suspend(struct device *dev)
 
 	netif_device_detach(ndev);
 
-	if (priv->wol_enabled)
-		return ravb_wol_setup(ndev);
+	rtnl_lock();
+	if (priv->wol_enabled) {
+		ret = ravb_wol_setup(ndev);
+		rtnl_unlock();
+		return ret;
+	}
 
 	ret = ravb_close(ndev);
+	rtnl_unlock();
 	if (ret)
 		return ret;
 
@@ -3245,19 +3250,25 @@ static int ravb_resume(struct device *dev)
 	if (!netif_running(ndev))
 		return 0;
 
+	rtnl_lock();
 	/* If WoL is enabled restore the interface. */
 	if (priv->wol_enabled) {
 		ret = ravb_wol_restore(ndev);
-		if (ret)
+		if (ret)  {
+			rtnl_unlock();
 			return ret;
+		}
 	} else {
 		ret = pm_runtime_force_resume(dev);
-		if (ret)
+		if (ret) {
+			rtnl_unlock();
 			return ret;
+		}
 	}
 
 	/* Reopening the interface will restore the device to the working state. */
 	ret = ravb_open(ndev);
+	rtnl_unlock();
 	if (ret < 0)
 		goto out_rpm_put;
 

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 ` [PATCH net 1/2] net: ravb: " Kory Maincent
@ 2025-01-22 17:22   ` Niklas Söderlund
  2025-01-22 18:33   ` Sergey Shtylyov
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Niklas Söderlund @ 2025-01-22 17:22 UTC (permalink / raw)
  To: Kory Maincent
  Cc: Paul Barker, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov, Sergei Shtylyov,
	Thomas Petazzoni, Claudiu Beznea, netdev, linux-renesas-soc,
	linux-kernel

Hi Kory,

Thanks for your work.

On 2025-01-22 17:19:28 +0100, Kory Maincent wrote:
> Fix the suspend path by ensuring the rtnl lock is held where required.
> Calls to ravb_open, ravb_close and wol operations must be performed under
> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> Without this fix, the following warning is triggered:
> [   39.032969] =============================
> [   39.032983] WARNING: suspicious RCU usage
> [   39.033019] -----------------------------
> [   39.033033] drivers/net/phy/phy_device.c:2004 suspicious
> rcu_dereference_protected() usage!
> ...
> [   39.033597] stack backtrace:
> [   39.033613] CPU: 0 UID: 0 PID: 174 Comm: python3 Not tainted
> 6.13.0-rc7-next-20250116-arm64-renesas-00002-g35245dfdc62c #7
> [   39.033623] Hardware name: Renesas SMARC EVK version 2 based on
> r9a08g045s33 (DT)
> [   39.033628] Call trace:
> [   39.033633]  show_stack+0x14/0x1c (C)
> [   39.033652]  dump_stack_lvl+0xb4/0xc4
> [   39.033664]  dump_stack+0x14/0x1c
> [   39.033671]  lockdep_rcu_suspicious+0x16c/0x22c
> [   39.033682]  phy_detach+0x160/0x190
> [   39.033694]  phy_disconnect+0x40/0x54
> [   39.033703]  ravb_close+0x6c/0x1cc
> [   39.033714]  ravb_suspend+0x48/0x120
> [   39.033721]  dpm_run_callback+0x4c/0x14c
> [   39.033731]  device_suspend+0x11c/0x4dc
> [   39.033740]  dpm_suspend+0xdc/0x214
> [   39.033748]  dpm_suspend_start+0x48/0x60
> [   39.033758]  suspend_devices_and_enter+0x124/0x574
> [   39.033769]  pm_suspend+0x1ac/0x274
> [   39.033778]  state_store+0x88/0x124
> [   39.033788]  kobj_attr_store+0x14/0x24
> [   39.033798]  sysfs_kf_write+0x48/0x6c
> [   39.033808]  kernfs_fop_write_iter+0x118/0x1a8
> [   39.033817]  vfs_write+0x27c/0x378
> [   39.033825]  ksys_write+0x64/0xf4
> [   39.033833]  __arm64_sys_write+0x18/0x20
> [   39.033841]  invoke_syscall+0x44/0x104
> [   39.033852]  el0_svc_common.constprop.0+0xb4/0xd4
> [   39.033862]  do_el0_svc+0x18/0x20
> [   39.033870]  el0_svc+0x3c/0xf0
> [   39.033880]  el0t_64_sync_handler+0xc0/0xc4
> [   39.033888]  el0t_64_sync+0x154/0x158
> [   39.041274] ravb 11c30000.ethernet eth0: Link is Down
> 
> Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

I need to apply [1] to see the WARNING: suspicious RCU usage splat at 
all, I guess there is a WARN_ONCE somewhere. But with this patch applied 
the splat is gone when resuming and WoL works.

Tested on R-Car M3N with NFS root on the interface used for WoL.

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

1. [PATCH] gpio: rcar: Use raw_spinlock to protect register access

> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index bc395294a32d..2c6d8e4966c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3217,10 +3217,15 @@ static int ravb_suspend(struct device *dev)
>  
>  	netif_device_detach(ndev);
>  
> -	if (priv->wol_enabled)
> -		return ravb_wol_setup(ndev);
> +	rtnl_lock();
> +	if (priv->wol_enabled) {
> +		ret = ravb_wol_setup(ndev);
> +		rtnl_unlock();
> +		return ret;
> +	}
>  
>  	ret = ravb_close(ndev);
> +	rtnl_unlock();
>  	if (ret)
>  		return ret;
>  
> @@ -3245,19 +3250,25 @@ static int ravb_resume(struct device *dev)
>  	if (!netif_running(ndev))
>  		return 0;
>  
> +	rtnl_lock();
>  	/* If WoL is enabled restore the interface. */
>  	if (priv->wol_enabled) {
>  		ret = ravb_wol_restore(ndev);
> -		if (ret)
> +		if (ret)  {
> +			rtnl_unlock();
>  			return ret;
> +		}
>  	} else {
>  		ret = pm_runtime_force_resume(dev);
> -		if (ret)
> +		if (ret) {
> +			rtnl_unlock();
>  			return ret;
> +		}
>  	}
>  
>  	/* Reopening the interface will restore the device to the working state. */
>  	ret = ravb_open(ndev);
> +	rtnl_unlock();
>  	if (ret < 0)
>  		goto out_rpm_put;
>  
> 
> -- 
> 2.34.1
> 

-- 
Kind Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 ` [PATCH net 1/2] net: ravb: " Kory Maincent
  2025-01-22 17:22   ` Niklas Söderlund
@ 2025-01-22 18:33   ` Sergey Shtylyov
  2025-01-22 18:53     ` Sergey Shtylyov
  2025-01-23  9:30   ` Paul Barker
  2025-01-23 11:33   ` Claudiu Beznea
  3 siblings, 1 reply; 13+ messages in thread
From: Sergey Shtylyov @ 2025-01-22 18:33 UTC (permalink / raw)
  To: Kory Maincent, Paul Barker, Niklas Söderlund, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Mikhail Ulyanov
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel

Hello!

  My Cogent Embedded tenure is long over, so I dropped my old email... :-)

On 1/22/25 7:19 PM, Kory Maincent wrote:

> Fix the suspend path by ensuring the rtnl lock is held where required.

   Maybe suspend/resume path (the same w/the subject)?

> Calls to ravb_open, ravb_close and wol operations must be performed under
> the rtnl lock to prevent conflicts with ongoing ndo operations.

[...]

> Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

    FWIW:

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index bc395294a32d..2c6d8e4966c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -3245,19 +3250,25 @@ static int ravb_resume(struct device *dev)
>  	if (!netif_running(ndev))
>  		return 0;
>  
> +	rtnl_lock();
>  	/* If WoL is enabled restore the interface. */
>  	if (priv->wol_enabled) {
>  		ret = ravb_wol_restore(ndev);
> -		if (ret)
> +		if (ret)  {
> +			rtnl_unlock();
>  			return ret;
> +		}
>  	} else {
>  		ret = pm_runtime_force_resume(dev);
> -		if (ret)
> +		if (ret) {
> +			rtnl_unlock();
>  			return ret;

   Hm, are you sure we need to have rtnl_lock around pm_runtime_force_resume() too?

[...]

MBR, Sergey


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-22 18:33   ` Sergey Shtylyov
@ 2025-01-22 18:53     ` Sergey Shtylyov
  0 siblings, 0 replies; 13+ messages in thread
From: Sergey Shtylyov @ 2025-01-22 18:53 UTC (permalink / raw)
  To: Kory Maincent, Paul Barker, Niklas Söderlund, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Mikhail Ulyanov
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel

On 1/22/25 9:33 PM, Sergey Shtylyov wrote:
[...]

>> Fix the suspend path by ensuring the rtnl lock is held where required.
> 
>    Maybe suspend/resume path (the same w/the subject)?
> 
>> Calls to ravb_open, ravb_close and wol operations must be performed under
>> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> [...]
> 
>> Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
>> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
>> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> 
>     FWIW:
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index bc395294a32d..2c6d8e4966c3 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -3245,19 +3250,25 @@ static int ravb_resume(struct device *dev)
>>  	if (!netif_running(ndev))
>>  		return 0;
>>  
>> +	rtnl_lock();
>>  	/* If WoL is enabled restore the interface. */
>>  	if (priv->wol_enabled) {
>>  		ret = ravb_wol_restore(ndev);
>> -		if (ret)
>> +		if (ret)  {
>> +			rtnl_unlock();
>>  			return ret;
>> +		}
>>  	} else {
>>  		ret = pm_runtime_force_resume(dev);
>> -		if (ret)
>> +		if (ret) {
>> +			rtnl_unlock();
>>  			return ret;
> 
>    Hm, are you sure we need to have rtnl_lock around pm_runtime_force_resume() too?
 
   Anyway, the above *if* statements are needlessly duplicated, I think it's time
that we do away with this! :-)

[...]

MBR, Sergey


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 ` [PATCH net 1/2] net: ravb: " Kory Maincent
  2025-01-22 17:22   ` Niklas Söderlund
  2025-01-22 18:33   ` Sergey Shtylyov
@ 2025-01-23  9:30   ` Paul Barker
  2025-01-23 11:33   ` Claudiu Beznea
  3 siblings, 0 replies; 13+ messages in thread
From: Paul Barker @ 2025-01-23  9:30 UTC (permalink / raw)
  To: Kory Maincent, Niklas Söderlund, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Mikhail Ulyanov, Sergei Shtylyov
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel


[-- Attachment #1.1.1: Type: text/plain, Size: 2631 bytes --]

On 22/01/2025 16:19, Kory Maincent wrote:
> Fix the suspend path by ensuring the rtnl lock is held where required.
> Calls to ravb_open, ravb_close and wol operations must be performed under
> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> Without this fix, the following warning is triggered:
> [   39.032969] =============================
> [   39.032983] WARNING: suspicious RCU usage
> [   39.033019] -----------------------------
> [   39.033033] drivers/net/phy/phy_device.c:2004 suspicious
> rcu_dereference_protected() usage!
> ...
> [   39.033597] stack backtrace:
> [   39.033613] CPU: 0 UID: 0 PID: 174 Comm: python3 Not tainted
> 6.13.0-rc7-next-20250116-arm64-renesas-00002-g35245dfdc62c #7
> [   39.033623] Hardware name: Renesas SMARC EVK version 2 based on
> r9a08g045s33 (DT)
> [   39.033628] Call trace:
> [   39.033633]  show_stack+0x14/0x1c (C)
> [   39.033652]  dump_stack_lvl+0xb4/0xc4
> [   39.033664]  dump_stack+0x14/0x1c
> [   39.033671]  lockdep_rcu_suspicious+0x16c/0x22c
> [   39.033682]  phy_detach+0x160/0x190
> [   39.033694]  phy_disconnect+0x40/0x54
> [   39.033703]  ravb_close+0x6c/0x1cc
> [   39.033714]  ravb_suspend+0x48/0x120
> [   39.033721]  dpm_run_callback+0x4c/0x14c
> [   39.033731]  device_suspend+0x11c/0x4dc
> [   39.033740]  dpm_suspend+0xdc/0x214
> [   39.033748]  dpm_suspend_start+0x48/0x60
> [   39.033758]  suspend_devices_and_enter+0x124/0x574
> [   39.033769]  pm_suspend+0x1ac/0x274
> [   39.033778]  state_store+0x88/0x124
> [   39.033788]  kobj_attr_store+0x14/0x24
> [   39.033798]  sysfs_kf_write+0x48/0x6c
> [   39.033808]  kernfs_fop_write_iter+0x118/0x1a8
> [   39.033817]  vfs_write+0x27c/0x378
> [   39.033825]  ksys_write+0x64/0xf4
> [   39.033833]  __arm64_sys_write+0x18/0x20
> [   39.033841]  invoke_syscall+0x44/0x104
> [   39.033852]  el0_svc_common.constprop.0+0xb4/0xd4
> [   39.033862]  do_el0_svc+0x18/0x20
> [   39.033870]  el0_svc+0x3c/0xf0
> [   39.033880]  el0t_64_sync_handler+0xc0/0xc4
> [   39.033888]  el0t_64_sync+0x154/0x158
> [   39.041274] ravb 11c30000.ethernet eth0: Link is Down
> 
> Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

I think we can simplify ravb_suspend() and ravb_resume() with a bit of
refactoring, but that can be done as a follow-up.

Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com>

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 ` [PATCH net 1/2] net: ravb: " Kory Maincent
                     ` (2 preceding siblings ...)
  2025-01-23  9:30   ` Paul Barker
@ 2025-01-23 11:33   ` Claudiu Beznea
  2025-01-23 14:08     ` Kory Maincent
  3 siblings, 1 reply; 13+ messages in thread
From: Claudiu Beznea @ 2025-01-23 11:33 UTC (permalink / raw)
  To: Kory Maincent, Paul Barker, Niklas Söderlund, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Mikhail Ulyanov, Sergei Shtylyov
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel

Hi, Kory,

On 22.01.2025 18:19, Kory Maincent wrote:
> Fix the suspend path by ensuring the rtnl lock is held where required.
> Calls to ravb_open, ravb_close and wol operations must be performed under
> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> Without this fix, the following warning is triggered:
> [   39.032969] =============================
> [   39.032983] WARNING: suspicious RCU usage
> [   39.033019] -----------------------------
> [   39.033033] drivers/net/phy/phy_device.c:2004 suspicious
> rcu_dereference_protected() usage!
> ...
> [   39.033597] stack backtrace:
> [   39.033613] CPU: 0 UID: 0 PID: 174 Comm: python3 Not tainted
> 6.13.0-rc7-next-20250116-arm64-renesas-00002-g35245dfdc62c #7
> [   39.033623] Hardware name: Renesas SMARC EVK version 2 based on
> r9a08g045s33 (DT)
> [   39.033628] Call trace:
> [   39.033633]  show_stack+0x14/0x1c (C)
> [   39.033652]  dump_stack_lvl+0xb4/0xc4
> [   39.033664]  dump_stack+0x14/0x1c
> [   39.033671]  lockdep_rcu_suspicious+0x16c/0x22c
> [   39.033682]  phy_detach+0x160/0x190
> [   39.033694]  phy_disconnect+0x40/0x54
> [   39.033703]  ravb_close+0x6c/0x1cc
> [   39.033714]  ravb_suspend+0x48/0x120
> [   39.033721]  dpm_run_callback+0x4c/0x14c
> [   39.033731]  device_suspend+0x11c/0x4dc
> [   39.033740]  dpm_suspend+0xdc/0x214
> [   39.033748]  dpm_suspend_start+0x48/0x60
> [   39.033758]  suspend_devices_and_enter+0x124/0x574
> [   39.033769]  pm_suspend+0x1ac/0x274
> [   39.033778]  state_store+0x88/0x124
> [   39.033788]  kobj_attr_store+0x14/0x24
> [   39.033798]  sysfs_kf_write+0x48/0x6c
> [   39.033808]  kernfs_fop_write_iter+0x118/0x1a8
> [   39.033817]  vfs_write+0x27c/0x378
> [   39.033825]  ksys_write+0x64/0xf4
> [   39.033833]  __arm64_sys_write+0x18/0x20
> [   39.033841]  invoke_syscall+0x44/0x104
> [   39.033852]  el0_svc_common.constprop.0+0xb4/0xd4
> [   39.033862]  do_el0_svc+0x18/0x20
> [   39.033870]  el0_svc+0x3c/0xf0
> [   39.033880]  el0t_64_sync_handler+0xc0/0xc4
> [   39.033888]  el0t_64_sync+0x154/0x158
> [   39.041274] ravb 11c30000.ethernet eth0: Link is Down
> 
> Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

I've test it. Looks good.

Thank you for your patch. However, I think this could be simplified. The
locking scheme looks complicated to me. E.g., this one works too:

diff --git a/drivers/net/ethernet/renesas/ravb_main.c
b/drivers/net/ethernet/renesas/ravb_main.c
index bc395294a32d..cfe4f0f364f3 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -3217,10 +3217,16 @@ static int ravb_suspend(struct device *dev)

        netif_device_detach(ndev);

-       if (priv->wol_enabled)
-               return ravb_wol_setup(ndev);
+       if (priv->wol_enabled) {
+               rtnl_lock();
+               ret = ravb_wol_setup(ndev);
+               rtnl_unlock();
+               return ret;
+       }

+       rtnl_lock();
        ret = ravb_close(ndev);
+       rtnl_unlock();
        if (ret)
                return ret;

@@ -3247,7 +3253,9 @@ static int ravb_resume(struct device *dev)

        /* If WoL is enabled restore the interface. */
        if (priv->wol_enabled) {
+               rtnl_lock();
                ret = ravb_wol_restore(ndev);
+               rtnl_unlock();
                if (ret)
                        return ret;
        } else {
@@ -3257,7 +3265,9 @@ static int ravb_resume(struct device *dev)
        }

        /* Reopening the interface will restore the device to the working
state. */
+       rtnl_lock();
        ret = ravb_open(ndev);
+       rtnl_unlock();
        if (ret < 0)
                goto out_rpm_put;


> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index bc395294a32d..2c6d8e4966c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3217,10 +3217,15 @@ static int ravb_suspend(struct device *dev)
>  
>  	netif_device_detach(ndev);
>  
> -	if (priv->wol_enabled)
> -		return ravb_wol_setup(ndev);
> +	rtnl_lock();
> +	if (priv->wol_enabled) {
> +		ret = ravb_wol_setup(ndev);
> +		rtnl_unlock();
> +		return ret;
> +	}
>  
>  	ret = ravb_close(ndev);
> +	rtnl_unlock();
>  	if (ret)
>  		return ret;
>  
> @@ -3245,19 +3250,25 @@ static int ravb_resume(struct device *dev)
>  	if (!netif_running(ndev))
>  		return 0;
>  
> +	rtnl_lock();
>  	/* If WoL is enabled restore the interface. */
>  	if (priv->wol_enabled) {
>  		ret = ravb_wol_restore(ndev);
> -		if (ret)
> +		if (ret)  {
> +			rtnl_unlock();
>  			return ret;
> +		}
>  	} else {
>  		ret = pm_runtime_force_resume(dev);
> -		if (ret)
> +		if (ret) {
> +			rtnl_unlock();
>  			return ret;
> +		}
>  	}
>  
>  	/* Reopening the interface will restore the device to the working state. */
>  	ret = ravb_open(ndev);
> +	rtnl_unlock();
>  	if (ret < 0)
>  		goto out_rpm_put;
>  
> 


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-23 11:33   ` Claudiu Beznea
@ 2025-01-23 14:08     ` Kory Maincent
  2025-01-23 14:17       ` Claudiu Beznea
  0 siblings, 1 reply; 13+ messages in thread
From: Kory Maincent @ 2025-01-23 14:08 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: Paul Barker, Niklas Söderlund, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov,
	Sergei Shtylyov, Thomas Petazzoni, Niklas Söderlund,
	Claudiu Beznea, netdev, linux-renesas-soc, linux-kernel

On Thu, 23 Jan 2025 13:33:30 +0200
Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:

> Hi, Kory,
> 
> On 22.01.2025 18:19, Kory Maincent wrote:
> > Fix the suspend path by ensuring the rtnl lock is held where required.
> > Calls to ravb_open, ravb_close and wol operations must be performed under
> > the rtnl lock to prevent conflicts with ongoing ndo operations.

> 
> I've test it. Looks good.
> 
> Thank you for your patch. However, I think this could be simplified. The
> locking scheme looks complicated to me. E.g., this one works too:
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> b/drivers/net/ethernet/renesas/ravb_main.c
> index bc395294a32d..cfe4f0f364f3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3217,10 +3217,16 @@ static int ravb_suspend(struct device *dev)
> 
>         netif_device_detach(ndev);
> 
> -       if (priv->wol_enabled)
> -               return ravb_wol_setup(ndev);
> +       if (priv->wol_enabled) {
> +               rtnl_lock();
> +               ret = ravb_wol_setup(ndev);
> +               rtnl_unlock();
> +               return ret;
> +       }

What happen if wol_enabled flag changes it state between the rtnl_lock and the
if condition? We will be in the wrong path.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-23 14:08     ` Kory Maincent
@ 2025-01-23 14:17       ` Claudiu Beznea
  2025-01-23 14:38         ` Kory Maincent
  0 siblings, 1 reply; 13+ messages in thread
From: Claudiu Beznea @ 2025-01-23 14:17 UTC (permalink / raw)
  To: Kory Maincent
  Cc: Paul Barker, Niklas Söderlund, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov,
	Sergei Shtylyov, Thomas Petazzoni, Niklas Söderlund,
	Claudiu Beznea, netdev, linux-renesas-soc, linux-kernel



On 23.01.2025 16:08, Kory Maincent wrote:
> On Thu, 23 Jan 2025 13:33:30 +0200
> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> 
>> Hi, Kory,
>>
>> On 22.01.2025 18:19, Kory Maincent wrote:
>>> Fix the suspend path by ensuring the rtnl lock is held where required.
>>> Calls to ravb_open, ravb_close and wol operations must be performed under
>>> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
>>
>> I've test it. Looks good.
>>
>> Thank you for your patch. However, I think this could be simplified. The
>> locking scheme looks complicated to me. E.g., this one works too:
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index bc395294a32d..cfe4f0f364f3 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -3217,10 +3217,16 @@ static int ravb_suspend(struct device *dev)
>>
>>         netif_device_detach(ndev);
>>
>> -       if (priv->wol_enabled)
>> -               return ravb_wol_setup(ndev);
>> +       if (priv->wol_enabled) {
>> +               rtnl_lock();
>> +               ret = ravb_wol_setup(ndev);
>> +               rtnl_unlock();
>> +               return ret;
>> +       }
> 
> What happen if wol_enabled flag changes it state between the rtnl_lock and the
> if condition? We will be in the wrong path.

wol_enabled flag can't change in this suspend phase. The user space tasks
are fronzen when ravb_suspend() is called.

Thank you,
Claudiu

> 
> Regards,


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path
  2025-01-23 14:17       ` Claudiu Beznea
@ 2025-01-23 14:38         ` Kory Maincent
  0 siblings, 0 replies; 13+ messages in thread
From: Kory Maincent @ 2025-01-23 14:38 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: Paul Barker, Niklas Söderlund, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov,
	Sergei Shtylyov, Thomas Petazzoni, Niklas Söderlund,
	Claudiu Beznea, netdev, linux-renesas-soc, linux-kernel

On Thu, 23 Jan 2025 16:17:58 +0200
Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:

> On 23.01.2025 16:08, Kory Maincent wrote:
> > On Thu, 23 Jan 2025 13:33:30 +0200
> > Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >   
> >> Hi, Kory,
> >>
> >> On 22.01.2025 18:19, Kory Maincent wrote:  
>  [...]  
> >   
> >>
> >> I've test it. Looks good.
> >>
> >> Thank you for your patch. However, I think this could be simplified. The
> >> locking scheme looks complicated to me. E.g., this one works too:
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >> b/drivers/net/ethernet/renesas/ravb_main.c
> >> index bc395294a32d..cfe4f0f364f3 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -3217,10 +3217,16 @@ static int ravb_suspend(struct device *dev)
> >>
> >>         netif_device_detach(ndev);
> >>
> >> -       if (priv->wol_enabled)
> >> -               return ravb_wol_setup(ndev);
> >> +       if (priv->wol_enabled) {
> >> +               rtnl_lock();
> >> +               ret = ravb_wol_setup(ndev);
> >> +               rtnl_unlock();
> >> +               return ret;
> >> +       }  
> > 
> > What happen if wol_enabled flag changes it state between the rtnl_lock and
> > the if condition? We will be in the wrong path.  
> 
> wol_enabled flag can't change in this suspend phase. The user space tasks
> are fronzen when ravb_suspend() is called.

Oh ok, I don't now the suspend path but if it can't conflict we can got for your
proposition.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net 2/2] net: sh_eth: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 [PATCH net 0/2] Fix missing rtnl lock in suspend path Kory Maincent
  2025-01-22 16:19 ` [PATCH net 1/2] net: ravb: " Kory Maincent
@ 2025-01-22 16:19 ` Kory Maincent
  2025-01-22 17:12   ` Niklas Söderlund
  2025-01-22 18:39   ` Sergey Shtylyov
  1 sibling, 2 replies; 13+ messages in thread
From: Kory Maincent @ 2025-01-22 16:19 UTC (permalink / raw)
  To: Paul Barker, Niklas Söderlund, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov,
	Sergei Shtylyov
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel, Kory Maincent

Fix the suspend path by ensuring the rtnl lock is held where required.
Calls to sh_eth_close, sh_eth_open and wol operations must be performed
under the rtnl lock to prevent conflicts with ongoing ndo operations.

Fixes: b71af04676e9 ("sh_eth: add more PM methods")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 8887b8921009..5fc8027c92c7 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -3494,10 +3494,12 @@ static int sh_eth_suspend(struct device *dev)
 
 	netif_device_detach(ndev);
 
+	rtnl_lock();
 	if (mdp->wol_enabled)
 		ret = sh_eth_wol_setup(ndev);
 	else
 		ret = sh_eth_close(ndev);
+	rtnl_unlock();
 
 	return ret;
 }
@@ -3511,10 +3513,12 @@ static int sh_eth_resume(struct device *dev)
 	if (!netif_running(ndev))
 		return 0;
 
+	rtnl_lock();
 	if (mdp->wol_enabled)
 		ret = sh_eth_wol_restore(ndev);
 	else
 		ret = sh_eth_open(ndev);
+	rtnl_unlock();
 
 	if (ret < 0)
 		return ret;

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net 2/2] net: sh_eth: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 ` [PATCH net 2/2] net: sh_eth: " Kory Maincent
@ 2025-01-22 17:12   ` Niklas Söderlund
  2025-01-22 18:39   ` Sergey Shtylyov
  1 sibling, 0 replies; 13+ messages in thread
From: Niklas Söderlund @ 2025-01-22 17:12 UTC (permalink / raw)
  To: Kory Maincent
  Cc: Paul Barker, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mikhail Ulyanov, Sergei Shtylyov,
	Thomas Petazzoni, Claudiu Beznea, netdev, linux-renesas-soc,
	linux-kernel

Hi Kory,

Thanks for your work.

On 2025-01-22 17:19:29 +0100, Kory Maincent wrote:
> Fix the suspend path by ensuring the rtnl lock is held where required.
> Calls to sh_eth_close, sh_eth_open and wol operations must be performed
> under the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> Fixes: b71af04676e9 ("sh_eth: add more PM methods")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Tested on R-Car M2,

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/net/ethernet/renesas/sh_eth.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 8887b8921009..5fc8027c92c7 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -3494,10 +3494,12 @@ static int sh_eth_suspend(struct device *dev)
>  
>  	netif_device_detach(ndev);
>  
> +	rtnl_lock();
>  	if (mdp->wol_enabled)
>  		ret = sh_eth_wol_setup(ndev);
>  	else
>  		ret = sh_eth_close(ndev);
> +	rtnl_unlock();
>  
>  	return ret;
>  }
> @@ -3511,10 +3513,12 @@ static int sh_eth_resume(struct device *dev)
>  	if (!netif_running(ndev))
>  		return 0;
>  
> +	rtnl_lock();
>  	if (mdp->wol_enabled)
>  		ret = sh_eth_wol_restore(ndev);
>  	else
>  		ret = sh_eth_open(ndev);
> +	rtnl_unlock();
>  
>  	if (ret < 0)
>  		return ret;
> 
> -- 
> 2.34.1
> 

-- 
Kind Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net 2/2] net: sh_eth: Fix missing rtnl lock in suspend path
  2025-01-22 16:19 ` [PATCH net 2/2] net: sh_eth: " Kory Maincent
  2025-01-22 17:12   ` Niklas Söderlund
@ 2025-01-22 18:39   ` Sergey Shtylyov
  1 sibling, 0 replies; 13+ messages in thread
From: Sergey Shtylyov @ 2025-01-22 18:39 UTC (permalink / raw)
  To: Kory Maincent, Paul Barker, Niklas Söderlund, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: Thomas Petazzoni, Niklas Söderlund, Claudiu Beznea, netdev,
	linux-renesas-soc, linux-kernel

On 1/22/25 7:19 PM, Kory Maincent wrote:

> Fix the suspend path by ensuring the rtnl lock is held where required.

   Maybe suspend/resume path (the same w/the subject)?

> Calls to sh_eth_close, sh_eth_open and wol operations must be performed
> under the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> Fixes: b71af04676e9 ("sh_eth: add more PM methods")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

    FWIW:

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-01-23 14:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-22 16:19 [PATCH net 0/2] Fix missing rtnl lock in suspend path Kory Maincent
2025-01-22 16:19 ` [PATCH net 1/2] net: ravb: " Kory Maincent
2025-01-22 17:22   ` Niklas Söderlund
2025-01-22 18:33   ` Sergey Shtylyov
2025-01-22 18:53     ` Sergey Shtylyov
2025-01-23  9:30   ` Paul Barker
2025-01-23 11:33   ` Claudiu Beznea
2025-01-23 14:08     ` Kory Maincent
2025-01-23 14:17       ` Claudiu Beznea
2025-01-23 14:38         ` Kory Maincent
2025-01-22 16:19 ` [PATCH net 2/2] net: sh_eth: " Kory Maincent
2025-01-22 17:12   ` Niklas Söderlund
2025-01-22 18:39   ` Sergey Shtylyov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).