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

[<Conor.Dooley@microchip.com>]

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.

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