From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [RFC PATCH v1 01/10] xen/arm: make secondary gic
 init as notifier call
Date: Sun, 23 Mar 2014 14:38:20 +0000
Message-ID: <532EF1DC.6000203@linaro.org>
References: <1395238631-2024-1-git-send-email-vijay.kilari@gmail.com>	<1395238631-2024-2-git-send-email-vijay.kilari@gmail.com>	<532AE392.1080107@linaro.org>
	<CALicx6spfuZw7fXfkVFRjrbh2ikwxr85hJ6HgdrvZ=mcTGG2aQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CALicx6spfuZw7fXfkVFRjrbh2ikwxr85hJ6HgdrvZ=mcTGG2aQ@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Vijay Kilari <vijay.kilari@gmail.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Prasun Kapoor <Prasun.Kapoor@caviumnetworks.com>, Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>, xen-devel@lists.xen.org, Stefano Stabellini <stefano.stabellini@citrix.com>
List-Id: xen-devel@lists.xenproject.org

Hello Vijay,

On 22/03/14 08:16, Vijay Kilari wrote:
> On Thu, Mar 20, 2014 at 6:18 PM, Julien Grall <julien.grall@linaro.org> 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:
>>          gic_init_secondary_cpu();
>>          break;
>>       default:
>>          break;
>>       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 boot_phys_offset,
>>>
>>>       mmu_init_secondary_cpu();
>>>
>>> -    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 GIC
>     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?

Regards,

-- 
Julien Grall