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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 17 May 2012 14:28:30 +1000
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Li Zhong <zhong@...ux.vnet.ibm.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Paul Mackerras <paulus@...ba.org>,
	PowerPC email list <linuxppc-dev@...ts.ozlabs.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	deepthi@...ux.vnet.ibm.com
Subject: Re: [PATCH powerpc] fix a lockdep complaint in start_secondary

On Thu, 2012-05-17 at 12:01 +0800, Li Zhong wrote:
> This patch tries to fix following lockdep complaints:

 .../...

> pseries_notify_cpu_idle_add_cpu() actually does
> cpuidle_disable_device(), and then cpuidle_enable_device(), which
> releases and allocates the resources respectively. ( Also, all the data
> are cleared and reinitialized after this cycle). The problem here is:
> something like kzalloc(GFP_KERNEL), wait_for_completion() would have
> problems running here where irqs are still disabled. 

So yes, it looks definitely fishy. I don't have time to study cpuidle
today to check whether that's correct or not so I'm CCing Deepthi
Dharwar who did all that cpuidle work for pseries.

Deepthi, can you check whether that patch is correct ?

> Actually, cpuidle_enable_device() is called for each possible cpu when
> the driver is registered. So I don't think the resources needed to be
> released and allocated each time cpu becomes online. Something like
> cpuidle_reset_device() would be enough to clear and reinitialize the
> data.
> 
> However, after some studying of the data to be cleared, I think it's
> also reasonable to keep the previous data. For example: 
> 
> /sys/devices/system/cpu/cpu#/cpuidle/state#/usage 
>         the number of times this idle state has been entered
> /sys/devices/system/cpu/cpu#/cpuidle/state#/time
>         the amount of time spent in this idle state
> 
> So I think we could just remove the function call doing the
> disable/enable cycle:
> 
> Please correct me if I missed anything. 
> 
> Reported-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> Signed-off-by: Li Zhong <zhong@...ux.vnet.ibm.com>
> Tested-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/smp.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/smp.c
> b/arch/powerpc/platforms/pseries/smp.c
> index e16bb8d..71706bc 100644
> --- a/arch/powerpc/platforms/pseries/smp.c
> +++ b/arch/powerpc/platforms/pseries/smp.c
> @@ -147,7 +147,6 @@ static void __devinit smp_xics_setup_cpu(int cpu)
>  	set_cpu_current_state(cpu, CPU_STATE_ONLINE);
>  	set_default_offline_state(cpu);
>  #endif
> -	pseries_notify_cpuidle_add_cpu(cpu);
>  }
>  
>  static int __devinit smp_pSeries_kick_cpu(int nr)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists