+ Rolf Hi Arnd, Helge, Rolf, On Fri, 12 Oct 2018 at 17:21, Arnd Bergmann <arnd@xxxxxxxx> wrote: > > On Fri, Oct 12, 2018 at 11:45 AM Firoz Khan <firoz.khan@xxxxxxxxxx> wrote: > > > diff --git a/arch/parisc/kernel/syscalls/Makefile b/arch/parisc/kernel/syscalls/Makefile > > new file mode 100644 > > index 0000000..a0af5a3 > > --- /dev/null > > +++ b/arch/parisc/kernel/syscalls/Makefile > > > +syshdr_abi_unistd_32 := common,32 > > +syshdr_offset_unistd_32 := __NR_Linux > > +$(uapi)/unistd_32.h: $(syscall) $(syshdr) > > + $(call if_changed,syshdr) > > + > > +syshdr_abi_unistd_64 := common,64 > > +syshdr_offset_unistd_64 := __NR_Linux > > +$(uapi)/unistd_64.h: $(syscall) $(syshdr) > > + $(call if_changed,syshdr) > > The __NR_Linux seems misplaced here, don't we need that only for ia64 > and mips? No, It wasn't misplaced. you can refer below link. https://github.com/torvalds/linux/blob/master/arch/parisc/include/uapi/asm/unistd.h#L16 FYI, In IA64, I added the __NR_Linux to come up a generic .tbl file starts with 0 as a part system call table generation. I think you might be applied my IA64 patches locally sometimes before and now you might be confused. https://github.com/torvalds/linux/blob/master/arch/ia64/include/uapi/asm/unistd.h Yes, MIPS also uses this macro. > > > +systbl_abi_syscall_table_32 := common,32 > > +$(kapi)/syscall_table_32.h: $(syscall) $(systbl) > > + $(call if_changed,systbl) > > + > > +systbl_abi_syscall_table_64 := common,64 > > +$(kapi)/syscall_table_64.h: $(syscall) $(systbl) > > + $(call if_changed,systbl) > > Have you considered making the 'common' part implied? > I expected to see it done that way, although listing it explicitly > doesn't seem harmful either. It can't be done in that way easily, I see some problem there existing script. The problem you will understand by removing "common" and run the script. You can do diff before and after the generated files. ref: grep -E "^[0-9A-Fa-fXx]+[[:space:]]+${my_abis}" "$in" | sort -n | ( FYI, x86/arm/s390 implementation listing explicitly! So I almost followed there way of implementation. If you really want that way, please comment here. I need to redo the scripting for all 10 architecture. > > > +systbl_abi_syscall_table_c32 := common,compat,32 > > +$(kapi)/syscall_table_c32.h: $(syscall) $(systbl) > > + $(call if_changed,systbl) > > The way you specify 'compat' as one item in a list of > ABIs seems rather odd, I'd suggest keeping that a separate > flag. Commented below. > > Passing "common|32" as the list of ABIs in the first argument, > and 'compat' as the second argument. > > I think you can also pass arguments to 'if_changed', rather than > setting a global variable to control it. Sure. I'll have a look into this one! > arch/powerpc/boot/Makefile has some examples of that. > It should be possible to do this like > > $(kapi)/syscall_table_c32.h: $(syscall) $(systbl) > $(call if_changed,systbl,common|32,compat) This is something interesting! Rolf, I was trying to explain this one yesterday. Sorry, I know I haven't composed the mail properly. The uapi header generation script syscall table header generation script is invoked by this Makefile. systbl_abi_syscall_table_32 := common,32 $(kapi)/syscall_table_32.h: $(syscall) $(systbl) $(call if_changed,systbl) Here I want to generate systbl_abi_syscall_table_32, so I'll pass few args including the .tbl file. So script must have to identify that it is for 32. It has to read 4th column as <32/64 entry point> from the .tbl file. # The format is: # <number> <abi> <name> <32/64 entry point> <compat entry point> 5 common open sys_open compat_sys_open Similarly for 64 also. Same 4th column should have to read. systbl_abi_syscall_table_c32 := common,compat,32 $(kapi)/syscall_table_c32.h: $(syscall) $(systbl) $(call if_changed,systbl) But for compat interface either it has to read 5th column if present, otherwise 4th column. Script won't understand it is for compat unless we have to explicitly inform from Makefile. There are multiple way to do: 1. This implementation systbl_abi_syscall_table_c32 := common,compat,32 /* Makefile */ my_abi="$(cut -d'|' -f2 <<< $my_abis)" /*systbl.sh */ if [ $my_abi = "compat" ]; then if [ -z "$compat" ]; then emit $nxt $nr $entry else emit $nxt $nr $compat fi else emit $nxt $nr $entry fi 2. Add extra flag in the Makefile systbl_abi_syscall_table_c32 := common,32 systbl_xyz_syscall_table_c32 := compat $(kapi)/syscall_table_c32.h: $(syscall) $(systbl) $(call if_changed,systbl) and check from the script and identify it. This looks the direct method. Here I think the problem is adding one more args 3. Without Makefile change we can identify it. No need to add extra flag Makefile ------------ systbl_abi_syscall_table_c32 := common,32 $(kapi)/syscall_table_c32.h: $(syscall) $(systbl) $(call if_changed,systbl) systbl.sh ------------- if [ ${out: -5} = "c32.h" ]; then if [ -z "$compat" ]; then emit $nxt $nr $entry else emit $nxt $nr $compat fi elif [ ${out: -4} = "64.h" -o ${out: -4} = "32.h" ]; then emit $nxt $nr $entry fi Here I was asking is there any better way to do the same. Note: The name compat in Makefile may change to c32. Note: This implementation remain same for spark and powerpc hopefully. But Mips has extra one more interface. We need to consider that also here. > > > diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl > > new file mode 100644 > > index 0000000..7c9f268 > > --- /dev/null > > +++ b/arch/parisc/kernel/syscalls/syscall.tbl > ... > > +346 common copy_file_range sys_copy_file_range > > +347 common preadv2 sys_preadv2 compat_sys_preadv2 > > +348 common pwritev2 sys_pwritev2 compat_sys_pwritev2 > > +349 common statx sys_statx > > +350 common io_pgetevents sys_io_pgetevents compat_sys_io_pgetevents > > \ No newline at end of file > > Here is the missing newline again. This should never happen if your text > editor is configured correctly. > > > diff --git a/arch/parisc/kernel/syscalls/syscallhdr.sh b/arch/parisc/kernel/syscalls/syscallhdr.sh > > new file mode 100644 > > index 0000000..607d4ca > > --- /dev/null > > +++ b/arch/parisc/kernel/syscalls/syscallhdr.sh > > @@ -0,0 +1,35 @@ > > +#!/bin/sh > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +in="$1" > > +out="$2" > > +my_abis=`echo "($3)" | tr ',' '|'` > > +prefix="$4" > > +offset="$5" > > + > > +fileguard=_UAPI_ASM_PARISC_`basename "$out" | sed \ > > + -e 'y/abcdefghijklmnopqrstuvwxyz/ABCDEFGHIJKLMNOPQRSTUVWXYZ/' \ > > + -e 's/[^A-Z0-9_]/_/g' -e 's/__/_/g'` > > Maybe use ${ARCH} instead of PARISC here to keep it the same > across architectures? Sure. FYI, x86/arm/s390 has the above implementation, > > > + my_abi="$(cut -d'|' -f2 <<< $my_abis)" > > + while read nr abi name entry compat ; do > > + if [ $my_abi = "compat" ]; then > > This check seems really fragile, but if you add another argument to the > script instead of listing "compat" as the second option in the > list of ABIs, I think it's fine. Hmm. Please share ur comment in the above for the same. Thanks Firoz > > ARnd
