[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Murali N]

On Sun, Oct 13, 2013 at 11:02 AM, Yuanyuan ZHONG <zyy at motorola.com> wrote:
> On Fri, Oct 11, 2013 at 1:08 PM, Yuanyuan ZHONG <zyy at motorola.com> wrote:
>> On Fri, Oct 11, 2013 at 11:26 AM, Russell King - ARM Linux
>> <linux at arm.linux.org.uk> wrote:
>>> On Fri, Oct 11, 2013 at 11:12:19AM -0500, Yuanyuan ZHONG wrote:
>>>> On Fri, Oct 11, 2013 at 7:07 AM, Russell King - ARM Linux
>>>> <linux at arm.linux.org.uk> wrote:
>>>> > On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
>>>> >> Hi Russell, et al
>>>> >>
>>>> >> If there is no further comments, I'll submit it to patch system.
>>>> >> Thanks.
>>>> >
>>>> > No, I still don't agree with your patch as being the correct fix.  Look
>>>> > at the code that you're creating:
>>>> >
>>>> >         if (vfp_state_in_hw(cpu, thread)) {
>>>> >                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>>>> >                 vfp_current_hw_state[cpu] = NULL;
>>>> >         }
>>>> > #ifdef CONFIG_SMP
>>>> >         thread->vfpstate.hard.cpu = NR_CPUS;
>>>> >         vfp_current_hw_state[cpu] = NULL;
>>>> > #endif
>>>> >
>>>> > So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
>>>> > pointer for this CPU?
>>>>
>>>> I think ensuring local vfp_current_hw_state is invalidated is what
>>>> vfp_force_reload() need to do when SMP is enabled.
>>>
>>> vfp_force_reload() is about asking for the state S on CPU N to be
>>> reloaded.  It is not about merely asking for the state on CPU N to
>>> be reloaded.
>>
>> Then this function cannot do what we need for cpu hotplug. At that
>> time we don't know if the state S on CPU N is valid, we can only ask
>> for the state on CPU N to be reloaded. How about this:
>>
>> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
>> index 52b8f40..efdd9e0 100644
>> --- a/arch/arm/vfp/vfpmodule.c
>> +++ b/arch/arm/vfp/vfpmodule.c
>> @@ -644,6 +644,7 @@ static int vfp_hotplug(struct notifier_block *b,
>> unsigned long action,
>>  {
>>         if (action == CPU_DYING || action == CPU_DYING_FROZEN) {
>>                 vfp_force_reload((long)hcpu, current_thread_info());
>> +               vfp_current_hw_state[(long)hcpu] = NULL;
>>         } else if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
>>                 vfp_enable(NULL);
>>         return NOTIFY_OK;
>
> The vfp_force_reload() here does nothing. We can remove it and this this becomes
>
> static int vfp_hotplug(struct notifier_block *b, unsigned long action,
>         void *hcpu)
> {
>         if (action == CPU_DYING || action == CPU_DYING_FROZEN)
>                 vfp_current_hw_state[(long)hcpu] = NULL;
>         else if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
>                 vfp_enable(NULL);
>         return NOTIFY_OK;
> }
>

What is the final path going to be?
As per Russell it seems setting the "vfp_current_hw_state[cpu] =
NULL;" unconditionaly in "vfp_force_reload()" is wrong.

-- 
Regards,
Murali N