[PATCH] powerpc/imc-pmu: Revert nest_init_lock to being a mutex

[PATCH] powerpc/imc-pmu: Revert nest_init_lock to being a mutex

[Michael Ellerman]

The recent commit 76d588dddc45 ("powerpc/imc-pmu: Fix use of mutex in
IRQs disabled section") fixed warnings (and possible deadlocks) in the
IMC PMU driver by converting the locking to use spinlocks.

It also converted the init-time nest_init_lock to a spinlock, even
though it's not used at runtime in IRQ disabled sections or while
holding other spinlocks.

This leads to warnings such as:

  BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: swapper/0
  preempt_count: 1, expected: 0
  CPU: 7 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc2-14719-gf12cd06109f4-dirty #1
  Hardware name: Mambo,Simulated-System POWER9 0x4e1203 opal:v6.6.6 PowerNV
  Call Trace:
    dump_stack_lvl+0x74/0xa8 (unreliable)
    __might_resched+0x178/0x1a0
    __cpuhp_setup_state+0x64/0x1e0
    init_imc_pmu+0xe48/0x1250
    opal_imc_counters_probe+0x30c/0x6a0
    platform_probe+0x78/0x110
    really_probe+0x104/0x420
    __driver_probe_device+0xb0/0x170
    driver_probe_device+0x58/0x180
    __driver_attach+0xd8/0x250
    bus_for_each_dev+0xb4/0x140
    driver_attach+0x34/0x50
    bus_add_driver+0x1e8/0x2d0
    driver_register+0xb4/0x1c0
    __platform_driver_register+0x38/0x50
    opal_imc_driver_init+0x2c/0x40
    do_one_initcall+0x80/0x360
    kernel_init_freeable+0x310/0x3b8
    kernel_init+0x30/0x1a0
    ret_from_kernel_thread+0x5c/0x64

Fix it by converting nest_init_lock back to a mutex, so that we can call
sleeping functions while holding it. There is no interaction between
nest_init_lock and the runtime spinlocks used by the actual PMU routines.

Fixes: 76d588dddc45 ("powerpc/imc-pmu: Fix use of mutex in IRQs disabled section")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/perf/imc-pmu.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 100e97daf76b..9d229ef7f86e 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -22,7 +22,7 @@
  * Used to avoid races in counting the nest-pmu units during hotplug
  * register and unregister
  */
-static DEFINE_SPINLOCK(nest_init_lock);
+static DEFINE_MUTEX(nest_init_lock);
 static DEFINE_PER_CPU(struct imc_pmu_ref *, local_nest_imc_refc);
 static struct imc_pmu **per_nest_pmu_arr;
 static cpumask_t nest_imc_cpumask;
@@ -1629,7 +1629,7 @@ static void imc_common_mem_free(struct imc_pmu *pmu_ptr)
 static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
 {
 	if (pmu_ptr->domain == IMC_DOMAIN_NEST) {
-		spin_lock(&nest_init_lock);
+		mutex_lock(&nest_init_lock);
 		if (nest_pmus == 1) {
 			cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE);
 			kfree(nest_imc_refc);
@@ -1639,7 +1639,7 @@ static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
 
 		if (nest_pmus > 0)
 			nest_pmus--;
-		spin_unlock(&nest_init_lock);
+		mutex_unlock(&nest_init_lock);
 	}
 
 	/* Free core_imc memory */
@@ -1796,11 +1796,11 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
 		* rest. To handle the cpuhotplug callback unregister, we track
 		* the number of nest pmus in "nest_pmus".
 		*/
-		spin_lock(&nest_init_lock);
+		mutex_lock(&nest_init_lock);
 		if (nest_pmus == 0) {
 			ret = init_nest_pmu_ref();
 			if (ret) {
-				spin_unlock(&nest_init_lock);
+				mutex_unlock(&nest_init_lock);
 				kfree(per_nest_pmu_arr);
 				per_nest_pmu_arr = NULL;
 				goto err_free_mem;
@@ -1808,7 +1808,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
 			/* Register for cpu hotplug notification. */
 			ret = nest_pmu_cpumask_init();
 			if (ret) {
-				spin_unlock(&nest_init_lock);
+				mutex_unlock(&nest_init_lock);
 				kfree(nest_imc_refc);
 				kfree(per_nest_pmu_arr);
 				per_nest_pmu_arr = NULL;
@@ -1816,7 +1816,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
 			}
 		}
 		nest_pmus++;
-		spin_unlock(&nest_init_lock);
+		mutex_unlock(&nest_init_lock);
 		break;
 	case IMC_DOMAIN_CORE:
 		ret = core_imc_pmu_cpumask_init();
-- 
2.39.1

Re: [PATCH] powerpc/imc-pmu: Revert nest_init_lock to being a mutex

[kajoljain]

On 1/30/23 07:14, Michael Ellerman wrote:
> The recent commit 76d588dddc45 ("powerpc/imc-pmu: Fix use of mutex in
> IRQs disabled section") fixed warnings (and possible deadlocks) in the
> IMC PMU driver by converting the locking to use spinlocks.
> 
> It also converted the init-time nest_init_lock to a spinlock, even
> though it's not used at runtime in IRQ disabled sections or while
> holding other spinlocks.
> 
> This leads to warnings such as:
> 
>   BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49
>   in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: swapper/0
>   preempt_count: 1, expected: 0
>   CPU: 7 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc2-14719-gf12cd06109f4-dirty #1
>   Hardware name: Mambo,Simulated-System POWER9 0x4e1203 opal:v6.6.6 PowerNV
>   Call Trace:
>     dump_stack_lvl+0x74/0xa8 (unreliable)
>     __might_resched+0x178/0x1a0
>     __cpuhp_setup_state+0x64/0x1e0
>     init_imc_pmu+0xe48/0x1250
>     opal_imc_counters_probe+0x30c/0x6a0
>     platform_probe+0x78/0x110
>     really_probe+0x104/0x420
>     __driver_probe_device+0xb0/0x170
>     driver_probe_device+0x58/0x180
>     __driver_attach+0xd8/0x250
>     bus_for_each_dev+0xb4/0x140
>     driver_attach+0x34/0x50
>     bus_add_driver+0x1e8/0x2d0
>     driver_register+0xb4/0x1c0
>     __platform_driver_register+0x38/0x50
>     opal_imc_driver_init+0x2c/0x40
>     do_one_initcall+0x80/0x360
>     kernel_init_freeable+0x310/0x3b8
>     kernel_init+0x30/0x1a0
>     ret_from_kernel_thread+0x5c/0x64
> 
> Fix it by converting nest_init_lock back to a mutex, so that we can call
> sleeping functions while holding it. There is no interaction between
> nest_init_lock and the runtime spinlocks used by the actual PMU routines.

Thanks for the patch. Patch looks good to me.

Reviewed-by: Kajol Jain<kjain@linux.ibm.com>
Tested-by: Kajol Jain<kjain@linux.ibm.com>

Thanks,
Kajol Jain
> 
> Fixes: 76d588dddc45 ("powerpc/imc-pmu: Fix use of mutex in IRQs disabled section")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/perf/imc-pmu.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index 100e97daf76b..9d229ef7f86e 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -22,7 +22,7 @@
>   * Used to avoid races in counting the nest-pmu units during hotplug
>   * register and unregister
>   */
> -static DEFINE_SPINLOCK(nest_init_lock);
> +static DEFINE_MUTEX(nest_init_lock);
>  static DEFINE_PER_CPU(struct imc_pmu_ref *, local_nest_imc_refc);
>  static struct imc_pmu **per_nest_pmu_arr;
>  static cpumask_t nest_imc_cpumask;
> @@ -1629,7 +1629,7 @@ static void imc_common_mem_free(struct imc_pmu *pmu_ptr)
>  static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
>  {
>  	if (pmu_ptr->domain == IMC_DOMAIN_NEST) {
> -		spin_lock(&nest_init_lock);
> +		mutex_lock(&nest_init_lock);
>  		if (nest_pmus == 1) {
>  			cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE);
>  			kfree(nest_imc_refc);
> @@ -1639,7 +1639,7 @@ static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
>  
>  		if (nest_pmus > 0)
>  			nest_pmus--;
> -		spin_unlock(&nest_init_lock);
> +		mutex_unlock(&nest_init_lock);
>  	}
>  
>  	/* Free core_imc memory */
> @@ -1796,11 +1796,11 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>  		* rest. To handle the cpuhotplug callback unregister, we track
>  		* the number of nest pmus in "nest_pmus".
>  		*/
> -		spin_lock(&nest_init_lock);
> +		mutex_lock(&nest_init_lock);
>  		if (nest_pmus == 0) {
>  			ret = init_nest_pmu_ref();
>  			if (ret) {
> -				spin_unlock(&nest_init_lock);
> +				mutex_unlock(&nest_init_lock);
>  				kfree(per_nest_pmu_arr);
>  				per_nest_pmu_arr = NULL;
>  				goto err_free_mem;
> @@ -1808,7 +1808,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>  			/* Register for cpu hotplug notification. */
>  			ret = nest_pmu_cpumask_init();
>  			if (ret) {
> -				spin_unlock(&nest_init_lock);
> +				mutex_unlock(&nest_init_lock);
>  				kfree(nest_imc_refc);
>  				kfree(per_nest_pmu_arr);
>  				per_nest_pmu_arr = NULL;
> @@ -1816,7 +1816,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>  			}
>  		}
>  		nest_pmus++;
> -		spin_unlock(&nest_init_lock);
> +		mutex_unlock(&nest_init_lock);
>  		break;
>  	case IMC_DOMAIN_CORE:
>  		ret = core_imc_pmu_cpumask_init();

Re: [PATCH] powerpc/imc-pmu: Revert nest_init_lock to being a mutex

[Michael Ellerman]

On Mon, 30 Jan 2023 12:44:01 +1100, Michael Ellerman wrote:
> The recent commit 76d588dddc45 ("powerpc/imc-pmu: Fix use of mutex in
> IRQs disabled section") fixed warnings (and possible deadlocks) in the
> IMC PMU driver by converting the locking to use spinlocks.
> 
> It also converted the init-time nest_init_lock to a spinlock, even
> though it's not used at runtime in IRQ disabled sections or while
> holding other spinlocks.
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc/imc-pmu: Revert nest_init_lock to being a mutex
      https://git.kernel.org/powerpc/c/ad53db4acb415976761d7302f5b02e97f2bd097e

cheers