[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v1 01/10] xen/arm: make secondary gic init as notifier call

Hello Vijay,

On 22/03/14 08:16, Vijay Kilari wrote:
On Thu, Mar 20, 2014 at 6:18 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
+/* Set up the per-CPU parts of the GIC for a secondary CPU */
+static int  __cpuinit gic_init_secondary_cpu(struct notifier_block *nfb,
+                                        unsigned long action, void *hcpu)
+    if (action == CPU_STARTING)
+    {
+        spin_lock(&gic.lock);
+        gic_cpu_init();
+        gic_hyp_init();
+        spin_unlock(&gic.lock);
+    }
+    return NOTIFY_DONE;

This is not the correct way to create a notifier block.

You should have a good similar to (see an example in common/timer.c):

static cpu_callback(struct notifier_block* nfb,
                     unsigned long action, void *hcpu)
      unsigned int cpu = (unsigned long)hcpu;

      switch ( action )
      case CPU_STARTING:
      return NOTIFY_DONE;

Apart from __cpuinit, I could not see any difference with this implementation.
am I missing anything specific?

I'd prefer to use the same pattern function for CPU notifier (see common/timer.c) rather than creating a new one.

The main difference are:
- the function name: the callback will be called on different CPU state. The current name "gic_init_secondary_cpu" is too specific. We might want to extend this callback later. - switch case: It's easier to extend with a switch case compare to if (foo)

@@ -283,7 +283,7 @@ void __cpuinit start_secondary(unsigned long 


-    gic_init_secondary_cpu();
+    notify_cpu_starting(cpuid);

Can you explain why it's safe to move notify_cpu_starting earlier?

    When gic registers a notifier with action as CPU_STARTING, I am
getting a panic
    from gic driver because notify_cpu_starting() is called after most of the 
    initialization calls as below from start_secondary() are called.
Especially the issue is coming
    with init_maintenanc_interrupt(). So I have moved this notifier
before other GIC initialization
    calls and since I move notifier only before GIC initialization
calls it not be a problem.

It doesn't explain why it's safe... CPU_STARTING is also used in some place to initialize internal data structure. Are you sure that theses callbacks can be called earlier?


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.