Re: [PATCH 2/3] PowerPC/mpc85xx: Add hotplug support on E5500 and E500MC cores

On Tue, Aug 4, 2015 at 5:18 AM, Scott Wood [off-list ref] 
wrote:

[Added linuxppc-dev@lists.ozlabs.org.  Besides that list being 
required for
review of PPC patches, it feeds the patchwork that I use to track and 
apply
patches.]

On Mon, 2015-08-03 at 19:52 +0800, Chenhui Zhao wrote:

quoted

 On Sat, Aug 1, 2015 at 8:14 AM, Scott Wood [off-list ref]
 wrote:
 > On Fri, 2015-07-31 at 17:20 +0800,  b29983@freescale.comwrote:
 > >  From: Tang Yuantian [off-list ref]
 > >
 > >  Freescale E500MC and E5500 core-based platforms, like P4080, 
T1040,
 > >  support disabling/enabling CPU dynamically.
 > >  This patch adds this feature on those platforms.
 > >
 > >  Signed-off-by: Chenhui Zhao [off-list ref]
 > >  Signed-off-by: Tang Yuantian [off-list ref]

quoted

 +{
 > >  +     int i;
 > >  +
 > >  +     for (i = 0; i < 50000; i++) {
 > >  +             if (generic_check_cpu_dead(cpu)) {
 > >  +                     qoriq_pm_ops->cpu_die(cpu);
 > >  +#ifdef CONFIG_PPC64
 > >  +                     paca[cpu].cpu_start = 0;
 > >  +#endif
 > >  +                     return;
 > >  +             }
 > >  +             udelay(10);
 > >  +     }
 > >  +     pr_err("%s: CPU%d didn't die...\n", __func__, cpu);
 > >  +}
 >
 > Only 500ms timeout, versus 10sec in generic_cpu_die()?

 The process is fast. Maybe 10 second is too large.

Is it fast 100% of the time?  What if the CPU you intend to die is in 
a long
critical section?  What harm is there to having a longer timeout, 
similar to
what other platforms use?

Will change the max timeout to 10 seconds.

quoted

 >
 > >   #endif
 > >
 > >   static inline void flush_spin_table(void *spin_table)
 > >  @@ -246,11 +267,7 @@ static int smp_85xx_kick_cpu(int nr)
 > >                spin_table = phys_to_virt(*cpu_rel_addr);
 > >
 > >        local_irq_save(flags);
 > >  -#ifdef CONFIG_PPC32
 > >   #ifdef CONFIG_HOTPLUG_CPU
 > >  -     /* Corresponding to generic_set_cpu_dead() */
 > >  -     generic_set_cpu_up(nr);
 > >  -
 > >        if (system_state == SYSTEM_RUNNING) {
 > >                /*
 > >                 * To keep it compatible with old boot program 
which
 > > uses
 > >  @@ -263,6 +280,7 @@ static int smp_85xx_kick_cpu(int nr)
 > >                out_be32(&spin_table->addr_l, 0);
 > >                flush_spin_table(spin_table);
 > >
 > >  +             qoriq_pm_ops->cpu_up(nr);
 >
 > Again, is it possible to get here without a valid qoriq_pm_ops 
(i.e.
 > is there
 > anything stopping the user from trying to initiate CPU hotplug)?
 >
 > -Scott

 For every platform running this code, should has a valid 
qoriq_pm_ops.
 If not valid, it's a bug.

How do you prevent this code from running when there is no valid 
qoriq_pm_ops?

-Scott

Will check if qoriq_pm_ops is valid.

-Chenhui