Re: [PATCH] riscv/vdso: fix missing vdso_data declaration

[Ben Dooks <ben.dooks@sifive.com>]

On 15/09/2022 19:51, Conor.Dooley@microchip.com wrote:
> On 19/08/2022 08:07, Conor Dooley wrote:
>> On 19/08/2022 00:01, Palmer Dabbelt wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Sun, 14 Aug 2022 09:10:48 PDT (-0700), Conor.Dooley@microchip.com wrote:
>>>> On 11/08/2022 22:26, Conor.Dooley@microchip.com wrote:
>>>>> On 11/08/2022 22:06, Palmer Dabbelt wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> On Wed, 13 Jul 2022 23:34:36 PDT (-0700), Conor.Dooley@microchip.com wrote:
>>>>>>> On 13/07/2022 23:06, Ben Dooks wrote:
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>
>>>>>>>> The vdso_data is defined by has no declaration as the
>>>>>>>> arch/riscv/kernel/vdso.c does not include asm/vdso/vsyscall.h
>>>>>>>> for the definition. Fix the following sparse warning by
>>>>>>>> adding in the asm/vdso/vsyscall.h
>>>>>>>>
>>>>>>>> arch/riscv/kernel/vdso.c:42:18: warning: symbol 'vdso_data' was not declared. Should it be static?
>>>>>>>>
>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>>>>>
>>>>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>>>>
>>>>>> This one gives me a bunch of build errors, things like
>>>>>
>>>>> Hmm, what's your defconfig for this?
>>>>> I tested with my dev defconfig and riscv's default one at the time but
>>>>> saw nothing. Tried both again now & no errors.
>>>>
>>>> Reproduced it with riscv-systems-ci.
>>>> Looks like it is happening on one of your 32 bit defconfigs where
>>>> CONFIG_GENERIC_TIME_VSYSCALL is not set. In vdso.c, the include of
>>>> vdso/datapage.h is guarded by CONFIG_GENERIC_TIME_VSYSCALL but the
>>>> one in asm/vdso/vsyscall.h is not.
>>>>
>>>> Adding the same guard around the include there fixes the build /shrug
>>>
>>> Do you mind just sending a v2?  It's a touch easier than trying to
>>> squash stuff in.
>>>
>>
>> Ben sent the patch so I didn't(/don't) want to encroach by sending
>> a v2.
>>
>> Ben, FWIW what I did was:
>> diff --git a/arch/riscv/include/asm/vdso/vsyscall.h b/arch/riscv/include/asm/vdso/vsyscall.h
>> index 82fd5d83bd60..e82ce97d27d2 100644
>> --- a/arch/riscv/include/asm/vdso/vsyscall.h
>> +++ b/arch/riscv/include/asm/vdso/vsyscall.h
>> @@ -5,7 +5,10 @@
>>   #ifndef __ASSEMBLY__
>>   
>>   #include <linux/timekeeper_internal.h>
>> +
>> +#ifdef CONFIG_GENERIC_TIME_VSYSCALL
>>   #include <vdso/datapage.h>
>> +#endif
>>   
>>   extern struct vdso_data *vdso_data;
>>   
>> But I now I am not sure if that is the correct approach.
>> There's an ARCH_HAS_VDSO_DATA in vdso/datapage.h:
>>
>> #ifdef CONFIG_ARCH_HAS_VDSO_DATA
>> #include <asm/vdso/data.h>
>> #else
>> struct arch_vdso_data {};
>> #endif
>>
>> I did a little more digging since what we are currently doing is the
>> exact same as what arm64 does.. I spotted that in our Kconfig we:
>> select GENERIC_TIME_VSYSCALL if MMU && 64BIT
>>
>> Why does that depend on 64BIT? Should that have been updated when we
>> added the compat stuff? If that is turned on, I can trigger the build
>> error without applying Ben's patch.
>>
>> arm64 also has a compat syscall for __NR_gettimeofday. I don't know
>> anything about compat, so I cant contribute any reasons as to why we
>> don't have one, but to the untrained eye it seems like Ben's patch is
>> exposing a deficiency in our compat support that may not have been
>> noticed because we had GENERIC_TIME_SYSCALL disabled for 32BIT.
>>
>> Either way, I don't know enough about what's going on here to send an
>> actual patch, just enough to make some observations.
> 
> Hey Ben,
> Did you ever get around to looking at this again?
> Thanks,
> Conor.
> 

I think this got lost in the general list noise, if it still an issue
I'll go have a look this week.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv