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

[<Conor.Dooley@microchip.com>]

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.
Conor.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv