[GIT PULL] asm-generic: bugfix for asm/unistd.h
The following changes since commit 2e6e902d185027f8e3cb8b7305238f7e35d6a436: Linux 4.20-rc4 (2018-11-25 14:19:31 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git tags/asm-generic-4.20 for you to fetch changes up to b7d624ab431227af376787148cd7d474088c03aa: asm-generic: unistd.h: fixup broken macro include. (2018-12-06 16:57:47 +0100) asm-generic: bugfix for asm/unistd.h Multiple people reported a bug I introduced in asm-generic/unistd.h in 4.20, this is the obvious bugfix to get glibc and others to correctly build again on new architectures that no longer provide the old fstatat64() family of system calls. Guo Ren (1): asm-generic: unistd.h: fixup broken macro include. include/uapi/asm-generic/unistd.h | 4 1 file changed, 4 insertions(+)
Re: linux-next: bad merge in the y2038 tree
On Thu, Dec 6, 2018 at 9:12 PM Stephen Rothwell wrote: > > Hi Arnd, > > The y2038 tree contains a merge of next-20181206. I cannot use a tree > that includes a version of linux-next. I assume that this was just a > test merge, so please remove it. Fixed now, I had pushed the wrong branch after I had done the test build to make sure that it wouldn't conflict with anything in yesterday's linux-next. Sorry about that. Arnd
Re: [PATCH] asm-generic: unistd.h: fixup broken macro include.
On Thu, Dec 6, 2018 at 3:07 AM Guo Ren wrote: > > The broken macros make the glibc compile error. If there is no > __NR3264_fstat*, we should also removed related definitions. > > Signed-off-by: Guo Ren > Signed-off-by: Mao Han > Cc: Arnd Bergmann > --- Thanks for the reminder! Marcin had already sent me the same patch a while ago and I forgot to apply it. I applied your version now, which as a slightly better changelog, so it will be in linux-next tomorrow, and I plan to send it after that shows no regressions. Arnd > include/uapi/asm-generic/unistd.h | 4 > 1 file changed, 4 insertions(+) > > diff --git a/include/uapi/asm-generic/unistd.h > b/include/uapi/asm-generic/unistd.h > index 538546e..c7f3321 100644 > --- a/include/uapi/asm-generic/unistd.h > +++ b/include/uapi/asm-generic/unistd.h > @@ -760,8 +760,10 @@ __SYSCALL(__NR_rseq, sys_rseq) > #define __NR_ftruncate __NR3264_ftruncate > #define __NR_lseek __NR3264_lseek > #define __NR_sendfile __NR3264_sendfile > +#if defined(__ARCH_WANT_NEW_STAT) || defined(__ARCH_WANT_STAT64) > #define __NR_newfstatat __NR3264_fstatat > #define __NR_fstat __NR3264_fstat > +#endif > #define __NR_mmap __NR3264_mmap > #define __NR_fadvise64 __NR3264_fadvise64 > #ifdef __NR3264_stat > @@ -776,8 +778,10 @@ __SYSCALL(__NR_rseq, sys_rseq) > #define __NR_ftruncate64 __NR3264_ftruncate > #define __NR_llseek __NR3264_lseek > #define __NR_sendfile64 __NR3264_sendfile > +#if defined(__ARCH_WANT_NEW_STAT) || defined(__ARCH_WANT_STAT64) > #define __NR_fstatat64 __NR3264_fstatat > #define __NR_fstat64 __NR3264_fstat > +#endif > #define __NR_mmap2 __NR3264_mmap > #define __NR_fadvise64_64 __NR3264_fadvise64 > #ifdef __NR3264_stat > -- > 2.7.4 >
Re: [PATCH v5 1/6] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Dec 5, 2018 at 11:32 PM Sven Van Asbroeck wrote: > On Wed, Dec 5, 2018 at 2:17 PM Greg KH wrote: > > > > Great, then call it a 'fieldbus' class, not "fieldbus_dev' class. > > Small nit: > > Hardware connected to a fieldbus comes in two distinct flavours: > - clients (e.g. thermometer, robotic arm) called "fieldbus devices" > - servers (e.g. a PLC) called "fieldbus controllers" > > Their userspace APIs will probably differ quite a lot. > > The userspace API created by the patch is only for clients a.k.a. > "fieldbus devices". That's why I'm writing 'fieldbus_dev' all over the place. > > For simplicity, we could change that to just 'fieldbus'. But would this get > us in trouble when, at some point, we want to add a userspace API for > servers a.k.a. "fieldbus controllers" ? In the long run, would you expect to support more devices or more controllers that need a distinct driver? Whichever we have more of should probably get the shorter name. Arnd
[PATCH] powerpc: split compat syscall table out from native table
PowerPC uses a syscall table with native and compat calls interleaved,
which is a slightly simpler way to define two matching tables.
As we move to having the tables generated, that advantage is no longer
important, but the interleaved table gets in the way of using the
same scripts as on the other architectures.
Split out a new compat_sys_call_table symbol that contains all the
compat calls, and leave the main table for the native calls, to more
closely match the method we use everywhere else.
Signed-off-by: Arnd Bergmann
---
This is completely untested, it's just for illustration and to
get comments about whether this is a good idea.
Firoz, can you try to get this working?
---
arch/powerpc/include/asm/syscall.h | 3 +--
arch/powerpc/kernel/entry_64.S | 7 --
arch/powerpc/kernel/systbl.S | 37 --
arch/powerpc/kernel/vdso.c | 7 --
4 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/include/asm/syscall.h
b/arch/powerpc/include/asm/syscall.h
index ab9f3f0a8637..1a0e7a8b1c81 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -18,9 +18,8 @@
#include
/* ftrace syscalls requires exporting the sys_call_table */
-#ifdef CONFIG_FTRACE_SYSCALLS
extern const unsigned long sys_call_table[];
-#endif /* CONFIG_FTRACE_SYSCALLS */
+extern const unsigned long compat_sys_call_table[];
static inline int syscall_get_nr(struct task_struct *task, struct pt_regs
*regs)
{
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 7b1693adff2a..5574d92646f1 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -54,6 +54,9 @@
SYS_CALL_TABLE:
.tc sys_call_table[TC],sys_call_table
+COMPAT_SYS_CALL_TABLE:
+ .tc compat_sys_call_table[TC],compat_sys_call_table
+
/* This value is used to mark exception frames on the stack. */
exception_marker:
.tc ID_EXC_MARKER[TC],STACK_FRAME_REGS_MARKER
@@ -173,7 +176,7 @@ system_call:/* label this so stack
traces look sane */
ld r11,SYS_CALL_TABLE@toc(2)
andis. r10,r10,_TIF_32BIT@h
beq 15f
- addir11,r11,8 /* use 32-bit syscall entries */
+ ld r11,COMPAT_SYS_CALL_TABLE@toc(2)
clrldi r3,r3,32
clrldi r4,r4,32
clrldi r5,r5,32
@@ -181,7 +184,7 @@ system_call:/* label this so stack
traces look sane */
clrldi r7,r7,32
clrldi r8,r8,32
15:
- slwir0,r0,4
+ slwir0,r0,3
barrier_nospec_asm
/*
diff --git a/arch/powerpc/kernel/systbl.S b/arch/powerpc/kernel/systbl.S
index 919a32746ede..883b8e36964c 100644
--- a/arch/powerpc/kernel/systbl.S
+++ b/arch/powerpc/kernel/systbl.S
@@ -17,13 +17,13 @@
#include
#ifdef CONFIG_PPC64
-#define SYSCALL(func) .8byte DOTSYM(sys_##func),DOTSYM(sys_##func)
-#define COMPAT_SYS(func) .8byte
DOTSYM(sys_##func),DOTSYM(compat_sys_##func)
-#define PPC_SYS(func) .8byte DOTSYM(ppc_##func),DOTSYM(ppc_##func)
-#define OLDSYS(func) .8byte
DOTSYM(sys_ni_syscall),DOTSYM(sys_ni_syscall)
-#define SYS32ONLY(func).8byte
DOTSYM(sys_ni_syscall),DOTSYM(compat_sys_##func)
-#define PPC64ONLY(func).8byte
DOTSYM(ppc_##func),DOTSYM(sys_ni_syscall)
-#define SYSX(f, f3264, f32).8byte DOTSYM(f),DOTSYM(f3264)
+#define SYSCALL(func) .8byte DOTSYM(sys_##func)
+#define COMPAT_SYS(func) .8byte DOTSYM(sys_##func)
+#define PPC_SYS(func) .8byte DOTSYM(ppc_##func)
+#define OLDSYS(func) .8byte DOTSYM(sys_ni_syscall)
+#define SYS32ONLY(func).8byte DOTSYM(sys_ni_syscall)
+#define PPC64ONLY(func).8byte DOTSYM(ppc_##func)
+#define SYSX(f, f3264, f32).8byte DOTSYM(f)
#else
#define SYSCALL(func) .long sys_##func
#define COMPAT_SYS(func) .long sys_##func
@@ -48,3 +48,26 @@
sys_call_table:
#include
+
+#undef SYSCALL
+#undef COMPAT_SYS
+#undef PPC_SYS
+#undef OLDSYS
+#undef SYS32ONLY
+#undef PPC64ONLY
+#undef SYSX
+
+#ifdef CONFIG_COMPAT
+#define SYSCALL(func) .8byte DOTSYM(sys_##func)
+#define COMPAT_SYS(func) .8byte DOTSYM(compat_sys_##func)
+#define PPC_SYS(func) .8byte DOTSYM(ppc_##func)
+#define OLDSYS(func) .8byte DOTSYM(sys_ni_syscall)
+#define SYS32ONLY(func).8byte DOTSYM(compat_sys_##func)
+#define PPC64ONLY(func).8byte DOTSYM(sys_ni_syscall)
+#define SYSX(f, f3264, f32).8byte DOTSYM(f3264)
+
+.globl compat_sys_call_table
+compat_sys_call_table:
+
+#include
+#endif
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index 65b3bdb99f0b..7725a9714736 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -671,15 +671,18 @@ static void __init vdso_setup_syscall_map(void)
{
unsigned int i
Re: [PATCH 3/3] arm64: ftrace: add cond_resched() to func ftrace_make_(call|nop)
On Mon, Dec 3, 2018 at 8:22 PM Will Deacon wrote:
>
> Hi Anders,
>
> On Fri, Nov 30, 2018 at 04:09:56PM +0100, Anders Roxell wrote:
> > Both of those functions end up calling ftrace_modify_code(), which is
> > expensive because it changes the page tables and flush caches.
> > Microseconds add up because this is called in a loop for each dyn_ftrace
> > record, and this triggers the softlockup watchdog unless we let it sleep
> > occasionally.
> > Rework so that we call cond_resched() before going into the
> > ftrace_modify_code() function.
> >
> > Co-developed-by: Arnd Bergmann
> > Signed-off-by: Arnd Bergmann
> > Signed-off-by: Anders Roxell
> > ---
> > arch/arm64/kernel/ftrace.c | 10 ++
> > 1 file changed, 10 insertions(+)
>
> It sounds like you're running into issues with the existing code, but I'd
> like to understand a bit more about exactly what you're seeing. Which part
> of the ftrace patching is proving to be expensive?
>
> The page table manipulation only happens once per module when using PLTs,
> and the cache maintenance is just a single line per patch site without an
> IPI.
>
> Is it the loop in ftrace_replace_code() that is causing the hassle?
Yes: with an allmodconfig kernel, the ftrace selftest calls ftrace_replace_code
to look >4 through ftrace_make_call/ftrace_make_nop, and these
end up calling
static int __kprobes __aarch64_insn_write(void *addr, __le32 insn)
{
void *waddr = addr;
unsigned long flags = 0;
int ret;
raw_spin_lock_irqsave(_lock, flags);
waddr = patch_map(addr, FIX_TEXT_POKE0);
ret = probe_kernel_write(waddr, , AARCH64_INSN_SIZE);
patch_unmap(FIX_TEXT_POKE0);
raw_spin_unlock_irqrestore(_lock, flags);
return ret;
}
int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
{
u32 *tp = addr;
int ret;
/* A64 instructions must be word aligned */
if ((uintptr_t)tp & 0x3)
return -EINVAL;
ret = aarch64_insn_write(tp, insn);
if (ret == 0)
__flush_icache_range((uintptr_t)tp,
(uintptr_t)tp + AARCH64_INSN_SIZE);
return ret;
}
which seems to be where the main cost is. This is with inside of
qemu, and with lots of debugging options (in particular
kcov and ubsan) enabled, that make each function call
more expensive.
Arnd
Re: [PATCH] ARM: mmp/mmp2: fix cpu_is_mmp2() on mmp2-dt
On Sun, Dec 2, 2018 at 12:12 PM Lubomir Rintel wrote: > > cpu_is_mmp2() was equivalent to cpu_is_pj4(), wouldn't be correct for > multiplatform kernels. Fix it by also considering mmp_chip_id, as is > done for cpu_is_pxa168() and cpu_is_pxa910() above. > > Moreover, it is only available with CONFIG_CPU_MMP2 and thus doesn't work > on DT-based MMP2 machines. Enable it on CONFIG_MACH_MMP2_DT too. > > Note: CONFIG_CPU_MMP2 is only used for machines that use board files > instead of DT. It should perhaps be renamed. I'm not doing it now, because > I don't have a better idea. > > Signed-off-by: Lubomir Rintel Acked-by: Arnd Bergmann I'd suggest adding Cc: sta...@vger.kernel.org since this fixes bugs on other platforms when CONFIG_MMP2 is enabled in a multiplatform kernel.
Re: [PATCH v2] signal: add procfd_signal() syscall
On Sat, Dec 1, 2018 at 9:51 AM Arnd Bergmann wrote:
> On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski wrote:
> > On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann wrote:
> > > On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski wrote:
> > > > On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann wrote:
> > > > > siginfo_t as it is now still has a number of other downsides, and
> > > > > Andy in
> > > > > particular didn't like the idea of having three new variants on x86
> > > > > (depending on how you count). His alternative suggestion of having
> > > > > a single syscall entry point that takes a 'signfo_t __user *' but
> > > > > interprets
> > > > > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > > > > should work correctly, but feels wrong to me, or at least inconsistent
> > > > > with how we do this elsewhere.
>
> > > The '548 | 0x4000' part seems to be the only sensible
> > > way to handle x32 here. What exactly would you propose to
> > > avoid defining the other entry points?
> >
> > I would propose that it should be 335 | 0x4000. I can't see any
> > reasonable way to teach the kernel to reject 335 | 0x4000 that
> > wouldn't work just as well to accept it and make it do the right
> > thing. Currently we accept it and do the *wrong* thing, which is no
> > good.
I guess we could start with something like the change below, which
would unify the entry points for rt_{tg,}sigqueueinfo, so that
e.g. the 129 and 536 syscall numbers do the exact same thing, and
that would be the lp64 or ilp32 behavior, depending on the
0x4000 bit. For the new syscalls, we can then do the same
thing without assigning another number.
Arnd
diff --git a/arch/x86/entry/syscall_64.c b/arch/x86/entry/syscall_64.c
index d5252bc1e380..3233fb889a51 100644
--- a/arch/x86/entry/syscall_64.c
+++ b/arch/x86/entry/syscall_64.c
@@ -7,6 +7,11 @@
#include
#include
+#ifdef CONFIG_X86_X32_ABI
+#define __x64_sys_x86_rt_sigqueueinfo __x64_sys_rt_sigqueueinfo
+#define __x64_sys_x86_rt_tgsigqueueinfo __x64_sys_rt_tgsigqueueinfo
+#endif
+
/* this is a lie, but it does not hurt as sys_ni_syscall just returns
-EINVAL */
extern asmlinkage long sys_ni_syscall(const struct pt_regs *);
#define __SYSCALL_64(nr, sym, qual) extern asmlinkage long sym(const
struct pt_regs *);
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
b/arch/x86/entry/syscalls/syscall_64.tbl
index 0823eed2b02e..4a7393d34e03 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -137,7 +137,7 @@
126common capset __x64_sys_capset
12764 rt_sigpending __x64_sys_rt_sigpending
12864 rt_sigtimedwait __x64_sys_rt_sigtimedwait
-12964 rt_sigqueueinfo __x64_sys_rt_sigqueueinfo
+12964 rt_sigqueueinfo __x64_sys_x86_rt_sigqueueinfo
130common rt_sigsuspend __x64_sys_rt_sigsuspend
13164 sigaltstack __x64_sys_sigaltstack
132common utime __x64_sys_utime
@@ -305,7 +305,7 @@
294common inotify_init1 __x64_sys_inotify_init1
29564 preadv __x64_sys_preadv
29664 pwritev __x64_sys_pwritev
-29764 rt_tgsigqueueinfo __x64_sys_rt_tgsigqueueinfo
+29764 rt_tgsigqueueinfo __x64_sys_x86_rt_tgsigqueueinfo
298common perf_event_open __x64_sys_perf_event_open
29964 recvmmsg__x64_sys_recvmmsg
300common fanotify_init __x64_sys_fanotify_init
@@ -369,7 +369,7 @@
521x32 ptrace __x32_compat_sys_ptrace
522x32 rt_sigpending __x32_compat_sys_rt_sigpending
523x32 rt_sigtimedwait __x32_compat_sys_rt_sigtimedwait
-524x32 rt_sigqueueinfo __x32_compat_sys_rt_sigqueueinfo
+524x32 rt_sigqueueinfo __x64_sys_x86_rt_sigqueueinfo
525x32 sigaltstack __x32_compat_sys_sigaltstack
526x32 timer_create__x32_compat_sys_timer_create
527x32 mq_notify __x32_compat_sys_mq_notify
@@ -381,7 +381,7 @@
533x32 move_pages __x32_compat_sys_move_pages
534x32 preadv __x32_compat_sys_preadv64
535x32 pwritev __x32_compat_sys_pwritev64
-536x32 rt_tgsigqueueinfo __x32_compat_sys_rt_tgsigqueueinfo
+536x32 rt_tgsigqueueinfo __x64_sys_x86_rt_tgsigqueueinfo
537x32 recvmmsg__x32_compat_sys_recvmmsg
538x32 sendmmsg__x32_compat_sys_sendmmsg
539x32 process_vm_readv__x32_compat_sys_process_vm_readv
diff --git a/arch/x86/
Re: [PATCH v2] signal: add procfd_signal() syscall
On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski wrote:
> On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann wrote:
> > On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski wrote:
> > > On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann wrote:
> > > > siginfo_t as it is now still has a number of other downsides, and Andy
> > > > in
> > > > particular didn't like the idea of having three new variants on x86
> > > > (depending on how you count). His alternative suggestion of having
> > > > a single syscall entry point that takes a 'signfo_t __user *' but
> > > > interprets
> > > > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > > > should work correctly, but feels wrong to me, or at least inconsistent
> > > > with how we do this elsewhere.
> > The '548 | 0x4000' part seems to be the only sensible
> > way to handle x32 here. What exactly would you propose to
> > avoid defining the other entry points?
>
> I would propose that it should be 335 | 0x4000. I can't see any
> reasonable way to teach the kernel to reject 335 | 0x4000 that
> wouldn't work just as well to accept it and make it do the right
> thing. Currently we accept it and do the *wrong* thing, which is no
> good.
>
> > and we have to
> > add more complexity to the copy_siginfo_from_user()
> > implementation to duplicate the hack that exists in
> > copy_siginfo_from_user32().
>
> What hack are you referring to here?
I mean this part:
#ifdef CONFIG_COMPAT
int copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from)
#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION)
{
return __copy_siginfo_to_user32(to, from, in_x32_syscall());
}
int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from, bool x32_ABI)
#endif
{
...
case SIL_CHLD:
new.si_pid= from->si_pid;
new.si_uid= from->si_uid;
new.si_status = from->si_status;
#ifdef CONFIG_X86_X32_ABI
if (x32_ABI) {
new._sifields._sigchld_x32._utime = from->si_utime;
new._sifields._sigchld_x32._stime = from->si_stime;
} else
#endif
{
new.si_utime = from->si_utime;
new.si_stime = from->si_stime;
}
break;
...
}
#endif
If we have a '548 | 0x4000' entry pointing to
__x32_compat_sys_procfd_kill, then that will do the right
thing. If you instead try to have x32 call into the native
sys_procfd_kill, then copy_siginfo_to_user() will also have
to know about x32, effectively duplicating that mess above,
unless you want to also change all users of
copy_siginfo_to_user32() to use copy_siginfo_to_user()
and handle all cases in one function.
Arnd
Re: [PATCH v2] signal: add procfd_signal() syscall
On Sat, Dec 1, 2018 at 12:12 AM Arnd Bergmann wrote: > > On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione wrote: > > On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner > > wrote: > > > On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann > > > wrote: > > > > > > One humble point I would like to make is that what I care about most is a > > > sensible way forward without having to redo essential parts of how > > > syscalls work. > > > I don't want to introduce a sane, small syscall that ends up breaking all > > > over the place because we decided to fix past mistakes that technically > > > have nothing to do with the patch itself. > > > However, I do sympathize and understand these concerns. > > > > IMHO, it's fine to just replicate all the splits we have for the > > existing signal system calls. It's ugly, but once it's done, it'll be > > done for a long time. I can't see a need to add even more signal > > system calls after this one. > > We definitely need waitid_time64() and rt_sigtimedwait_time64() > in the very near future. To clarify: we probably don't need rt_sigtimedwait_time64() for x32, as it already has a 64-bit time_t. We might need waitid_time64() or something similar though, since the plan now is to change the time resolution for rusage to nanoseconds (__kernel_timespec) now. The exact behavior and name of waitid_time64() is still a matter of discussion. Arnd
Re: [PATCH v2] signal: add procfd_signal() syscall
On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione wrote: > On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner > wrote: > > On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann > > wrote: > > > > One humble point I would like to make is that what I care about most is a > > sensible way forward without having to redo essential parts of how syscalls > > work. > > I don't want to introduce a sane, small syscall that ends up breaking all > > over the place because we decided to fix past mistakes that technically > > have nothing to do with the patch itself. > > However, I do sympathize and understand these concerns. > > IMHO, it's fine to just replicate all the splits we have for the > existing signal system calls. It's ugly, but once it's done, it'll be > done for a long time. I can't see a need to add even more signal > system calls after this one. We definitely need waitid_time64() and rt_sigtimedwait_time64() in the very near future. Arnd
Re: [PATCH v2] signal: add procfd_signal() syscall
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski wrote:
>
> On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann wrote:
> > siginfo_t as it is now still has a number of other downsides, and Andy in
> > particular didn't like the idea of having three new variants on x86
> > (depending on how you count). His alternative suggestion of having
> > a single syscall entry point that takes a 'signfo_t __user *' but interprets
> > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > should work correctly, but feels wrong to me, or at least inconsistent
> > with how we do this elsewhere.
>
> If everyone else is okay with it, I can get on board with three
> variants on x86. What I can't get on board with is *five* variants on
> x86, which would be:
>
> procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
>
> syscall64 with nr == 335 (presumably): 64-bit
These seem unavoidable
> syscall64 with nr == 548 | 0x4000: x32
>
> syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
> true, behavior is arbitrary
>
> syscall64 with nr == 335 | 0x4000: x32 entry, but
> in_compat_syscall() == false, behavior is arbitrary
Am I misreading the code? The way I understand it, setting the
0x4000 bit means that both in_compat_syscall() and
in_x32_syscall become() true, based on
static inline bool in_x32_syscall(void)
{
#ifdef CONFIG_X86_X32_ABI
if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
return true;
#endif
return false;
}
The '548 | 0x4000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
> This mess isn't really Christian's fault -- it's been there for a
> while, but it's awful and I don't think we want to perpetuate it.
I'm not convinced that not assigning an x32 syscall number
improves the situation, it just means that we now have one
syscall that behaves completely differently from all others,
in that the x32 version requires being called through a
SYSCALL_DEFINE() entry point rather than a
COMPAT_SYSCALL_DEFINE() one, and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
Of course, the nicest option would be to completely remove
x32 so we can stop worrying about it.
Arnd
Re: [RFC PATCH 3/6] char: fastrpc: Add support for context Invoke method
On Fri, Nov 30, 2018 at 5:03 PM Srinivas Kandagatla
wrote:
> On 30/11/18 15:08, Arnd Bergmann wrote:
> > On Fri, Nov 30, 2018 at 4:01 PM Srinivas Kandagatla
> > wrote:
> >> Thanks Arnd for the review comments!
> >> On 30/11/18 13:41, Arnd Bergmann wrote:
> >>> On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
> >>> wrote:
> >
> >>>> +static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> >>>> +unsigned long arg)
> >>>> +{
> >>>> + struct fastrpc_user *fl = (struct fastrpc_user
> >>>> *)file->private_data;
> >>>> + struct fastrpc_channel_ctx *cctx = fl->cctx;
> >>>> + char __user *argp = (char __user *)arg;
> >>>> + int err;
> >>>> +
> >>>> + if (!fl->sctx) {
> >>>> + fl->sctx = fastrpc_session_alloc(cctx, 0);
> >>>> + if (!fl->sctx)
> >>>> + return -ENOENT;
> >>>> + }
> >>>
> >>> Shouldn't that session be allocated during open()?
> >>>
> >> Yes, and no, we do not need context in all the cases. In cases like we
> >> just want to allocate dmabuf.
> >
> > Can you give an example what that would be good for?
> >
>
> Currently the instance which does not need session is used as simple
> memory allocator (rpcmem), TBH, this is the side effect of trying to fit
> in with downstream application infrastructure which uses ion for andriod
> usecases.
That does not sound like enough of a reason then, user space is
easy to change here to just allocate the memory from the device itself.
The only reason that I can see for needing a dmabuf would be if
you have to share a buffer between two instances, and then you
can use either of them.
> >>>> +static void fastrpc_notify_users(struct fastrpc_user *user)
> >>>> +{
> >>>> + struct fastrpc_invoke_ctx *ctx, *n;will go
> >>>> +
> >>>> + spin_lock(>lock);
> >>>> + list_for_each_entry_safe(ctx, n, >pending, node)
> >>>> + complete(>work);
> >>>> + spin_unlock(>lock);
> >>>> +}
> >>>
> >>> Can you explain here what it means to have multiple 'users'
> >>> a 'fastrpc_user' structure? Why are they all done at the same time?
>
> user is allocated on every open(). Having multiple users means that
> there are more than one compute sessions running on a given dsp.
>
> They reason why all the users are notified here is because the dsp is
> going down, so all the compute sessions associated with it will not see
> any response from dsp, so any pending/waiting compute contexts are
> explicitly notified.
I don't get it yet. What are 'compute sessions'? Do you have
multiple threads running on a single instance at the same time?
I would have expected to only ever see one thread in the
'wait_for_completion()' above, and others possibly waiting
for a chance to get to but not already running.
> >> struct fastrpc_remote_crc {
> >> __u64 crc;
> >> __u64 reserved1
> >> __u64 reserved2
> >> __u64 reserved3
> >> };
> >
> > I don't see a need to add extra served fields for structures
> > that are already naturally aligned here, e.g. in
> > fastrpc_remote_arg we need the 'reserved1' but not
> > the 'reserved2'.
> Yes, I see, I overdone it!
> Other idea, is, may be I can try to combine these into single structure
> something like:
>
> struct fastrpc_invoke_arg {
> __u64 ptr;
> __u64 len;
> __u32 fd;
> __u32 reserved1
> __u64 attr;
> __u64 crc;
> };
>
> struct fastrpc_ioctl_invoke {
> __u32 handle;
> __u32 sc;
> /* The minimum size is scalar_length * 32*/
> struct fastrpc_invoke_args *args;
> };
That is still two structure, not one ;-)
> >> struct fastrpc_ioctl_invoke {
> >> __u32 handle;
> >> __u32 sc;
> >> /* The minimum size is scalar_length * 32 */
> >> struct fastrpc_remote_args *rargs;
> >> struct fastrpc_remote_fd *fds;
> >> struct fastrpc_remote_attr *attrs;
> >> struct fastrpc_remote_crc *crc;
> >> };
> >
> > Do these really have to be indirect then? Are they all
> > lists of variable length? How do you know how long?
> Yes, they are variable length and will be scalar length long.
> Scalar length is derived from sc variable in this structure.
Do you mean we have a variable number 'sc', but each array
always has the same length as the other ones? In that
case: yes, combining them seems sensible.
The other question this raises is: what is 'handle'?
Why is the file descriptor not enough to identify the
instance we want to talk to?
Arnd
Re: [RFC PATCH 3/6] char: fastrpc: Add support for context Invoke method
On Fri, Nov 30, 2018 at 4:01 PM Srinivas Kandagatla
wrote:
> Thanks Arnd for the review comments!
> On 30/11/18 13:41, Arnd Bergmann wrote:
> > On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
> > wrote:
> >> +static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> >> +unsigned long arg)
> >> +{
> >> + struct fastrpc_user *fl = (struct fastrpc_user
> >> *)file->private_data;
> >> + struct fastrpc_channel_ctx *cctx = fl->cctx;
> >> + char __user *argp = (char __user *)arg;
> >> + int err;
> >> +
> >> + if (!fl->sctx) {
> >> + fl->sctx = fastrpc_session_alloc(cctx, 0);
> >> + if (!fl->sctx)
> >> + return -ENOENT;
> >> + }
> >
> > Shouldn't that session be allocated during open()?
> >
> Yes, and no, we do not need context in all the cases. In cases like we
> just want to allocate dmabuf.
Can you give an example what that would be good for?
>
> >> +static void fastrpc_notify_users(struct fastrpc_user *user)
> >> +{
> >> + struct fastrpc_invoke_ctx *ctx, *n;
> >> +
> >> + spin_lock(>lock);
> >> + list_for_each_entry_safe(ctx, n, >pending, node)
> >> + complete(>work);
> >> + spin_unlock(>lock);
> >> +}
> >
> > Can you explain here what it means to have multiple 'users'
> > a 'fastrpc_user' structure? Why are they all done at the same time?
> >
> This is the case where users need to be notified if the dsp goes down
> due to crash or shut down!
What is a 'user' then? My point is that it seems to refer to two
different things here. I assume 'fastrpc_user' is whoever
has opened the file descriptor.
> >
> >> +struct fastrpc_ioctl_invoke {
> >> + uint32_t handle;/* remote handle */
> >> + uint32_t sc;/* scalars describing the data */
> >> + remote_arg_t *pra; /* remote arguments list */
> >> + int *fds; /* fd list */
> >> + unsigned int *attrs;/* attribute list */
> >> + unsigned int *crc;
> >> +};
> >
> > This seems too complex for an ioctl argument, with
> > multiple levels of pointer indirections. I'd normally
> > try to make each ioctl argument either a scalar, or a
> > structure with only fixed-length members.
> >
> I totally agree with you and many thanks for your expert inputs,
> May be something like below with fixed length members would work?
>
> struct fastrpc_remote_arg {
> __u64 ptr; /* buffer ptr */
> __u64 len; /* length */
> __u32 fd; /* dmabuf fd */
> __u32 reserved1
> __u64 reserved2
> };
>
> struct fastrpc_remote_fd {
> __u64 fd;
> __u64 reserved1
> __u64 reserved2
> __u64 reserved3
> };
>
> struct fastrpc_remote_attr {
> __u64 attr;
> __u64 reserved1
> __u64 reserved2
> __u64 reserved3
> };
>
> struct fastrpc_remote_crc {
> __u64 crc;
> __u64 reserved1
> __u64 reserved2
> __u64 reserved3
> };
I don't see a need to add extra served fields for structures
that are already naturally aligned here, e.g. in
fastrpc_remote_arg we need the 'reserved1' but not
the 'reserved2'.
>
> struct fastrpc_ioctl_invoke {
> __u32 handle;
> __u32 sc;
> /* The minimum size is scalar_length * 32 */
> struct fastrpc_remote_args *rargs;
> struct fastrpc_remote_fd *fds;
> struct fastrpc_remote_attr *attrs;
> struct fastrpc_remote_crc *crc;
> };
Do these really have to be indirect then? Are they all
lists of variable length? How do you know how long?
Arnd
Re: [PATCH 4/7] remoteproc: add warning on resource table cast
On Thu, Nov 29, 2018 at 10:31 PM Loic Pallardy wrote:
>
> Today resource table supports only 32bit address fields.
> This is not compliant with 64bit platform for which addresses
> are cast in 32bit.
> This patch adds warn messages when address cast is done.
>
> Signed-off-by: Loic Pallardy
> ---
> drivers/remoteproc/remoteproc_core.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 18a1bbf820c9..61c954bd695e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -772,6 +772,10 @@ static int rproc_alloc_carveout(struct rproc *rproc,
> dev_dbg(dev, "carveout mapped 0x%x to %pad\n",
> mem->da, );
> } else {
> + /* Update device address as undefined by requester */
> + if (sizeof(dma_addr_t) > sizeof(u32))
> + dev_warn(dev, "DMA address cast in 32bit to fit
> resource table format\n");
> +
> mem->da = (u32)dma;
> }
I think this patch is wrong: sizeof(dma_addr_t) is defined to be large enough
to support any machine that the kernel can run on. If you build a kernel that
happens to work on an ARM32 platform with LPAE enabled, then this will
warn every time, even if you are running on a machine that only has a 32-bit
address space.
Arnd
Re: [RFC PATCH 6/6] char: fastrpc: Add support for compat ioctls
On Fri, Nov 30, 2018 at 2:20 PM Thierry Escande wrote: > On 30/11/2018 13:58, Arnd Bergmann wrote: > > On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla > > wrote: > >> > >> From: Thierry Escande > >> > >> This patch adds support for compat ioctl from 32 bits userland to > >> Qualcomm fastrpc driver. > >> > >> Supported ioctls in this change are INIT, INVOKE, and ALLOC/FREE_DMA. > >> > >> Most of the work is derived from various downstream Qualcomm kernels. > >> Credits to various Qualcomm authors who have contributed to this code. > >> Specially Tharun Kumar Merugu > >> > >> Signed-off-by: Thierry Escande > >> Signed-off-by: Srinivas Kandagatla > > > > Can't you just define the native ioctls so that you don't need this. > > There are long time defined structures that are passed as argument to > these ioctls and their sizes vary between 32 and 64 bits userlands, so > the ioctl command values. Where? I don't see them in linux-4.19. > Unless I'm missing something here this also has the advantage not to be > compiled if CONFIG_COMPAT is not set. You can normally just set .compat_ioctl() to the same function as .unlocked_ioctl(), and get no overhead either way. Arnd
Re: [RFC PATCH 3/6] char: fastrpc: Add support for context Invoke method
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
wrote:
>
> This patch adds support to compute context invoke method
> on the remote processor (DSP).
> This involves setting up the functions input and output arguments,
> input and output handles and mapping the dmabuf fd for the
> argument/handle buffers.
>
> Most of the work is derived from various downstream Qualcomm kernels.
> Credits to various Qualcomm authors who have contributed to this code.
> Specially Tharun Kumar Merugu
>
> Signed-off-by: Srinivas Kandagatla
> +
> + INIT_LIST_HEAD(>node);
> + ctx->fl = user;
> + ctx->maps = (struct fastrpc_map **)([1]);
> + ctx->lpra = (remote_arg_t *)(>maps[bufs]);
> + ctx->fds = (int *)(>lpra[bufs]);
> + ctx->attrs = (unsigned int *)(>fds[bufs]);
> +
> + if (!kernel) {
> + if (copy_from_user(ctx->lpra,
> +(void const __user *)inv->pra,
> +bufs * sizeof(*ctx->lpra))) {
> + err = -EFAULT;
> + goto err;
> + }
> +
> + if (inv->fds) {
> + if (copy_from_user(ctx->fds,
> +(void const __user *)inv->fds,
> +bufs * sizeof(*ctx->fds))) {
> + err = -EFAULT;
> + goto err;
> + }
> + }
> + if (inv->attrs) {
> + if (copy_from_user(
> + ctx->attrs,
> + (void const __user *)inv->attrs,
> + bufs * sizeof(*ctx->attrs))) {
> + err = -EFAULT;
> + goto err;
> + }
> + }
> + } else {
> + memcpy(ctx->lpra, inv->pra, bufs * sizeof(*ctx->lpra));
> + if (inv->fds)
> + memcpy(ctx->fds, inv->fds,
> + bufs * sizeof(*ctx->fds));
> + if (inv->attrs)
> + memcpy(ctx->attrs, inv->attrs,
> + bufs * sizeof(*ctx->attrs));
> + }
I'd split this function into multiple pieces: the internal one that
just takes kernel pointers, and a wrapper for the ioctl
that copies the user space data into the kernel before calling
the second one.
> +static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> + uint32_t kernel, remote_arg_t *upra)
> +{
> + remote_arg64_t *rpra = ctx->rpra;
> + int i, inbufs, outbufs, handles;
> + struct fastrpc_invoke_buf *list;
> + struct fastrpc_phy_page *pages;
> + struct fastrpc_map *mmap;
> + uint32_t sc = ctx->sc;
> + uint64_t *fdlist;
> + uint32_t *crclist;
> + int err = 0;
> +
> + inbufs = REMOTE_SCALARS_INBUFS(sc);
> + outbufs = REMOTE_SCALARS_OUTBUFS(sc);
> + handles = REMOTE_SCALARS_INHANDLES(sc) +
> REMOTE_SCALARS_OUTHANDLES(sc);
> + list = fastrpc_invoke_buf_start(ctx->rpra, sc);
> + pages = fastrpc_phy_page_start(sc, list);
> + fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
> + crclist = (uint32_t *)(fdlist + FASTRPC_MAX_FDLIST);
> +
> + for (i = inbufs; i < inbufs + outbufs; ++i) {
> + if (!ctx->maps[i]) {
> + if (!kernel)
> + err =
> + copy_to_user((void __user
> *)ctx->lpra[i].buf.pv,
> + (void *)rpra[i].buf.pv,
> rpra[i].buf.len);
> + else
> + memcpy(ctx->lpra[i].buf.pv,
> + (void *)rpra[i].buf.pv,
> rpra[i].buf.len);
> +
> + if (err)
> + goto bail;
> + } else {
> + fastrpc_map_put(ctx->maps[i]);
> + ctx->maps[i] = NULL;
> + }
> + }
Same here.
> +static int fastrpc_internal_invoke(struct fastrpc_user *fl,
> + uint32_t kernel,
> + struct fastrpc_ioctl_invoke *inv)
> +{
> + struct fastrpc_invoke_ctx *ctx = NULL;
> + int err = 0;
> +
> + if (!fl->sctx)
> + return -EINVAL;
> +
> + ctx = fastrpc_context_alloc(fl, kernel, inv);
> + if (IS_ERR(ctx))
> + return PTR_ERR(ctx);
> +
> + if (REMOTE_SCALARS_LENGTH(ctx->sc)) {
> + err = fastrpc_get_args(kernel, ctx);
> + if (err)
> + goto bail;
> + }
> +
> + err = fastrpc_invoke_send(fl->sctx, ctx, kernel, inv->handle);
> + if (err)
> + goto bail;
> +
> + err =
Re: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
wrote:
> +
> +static int fastrpc_init_process(struct fastrpc_user *fl,
> + struct fastrpc_ioctl_init *init)
> +{
> + struct fastrpc_ioctl_invoke *ioctl;
> + struct fastrpc_phy_page pages[1];
> + struct fastrpc_map *file = NULL, *mem = NULL;
> + struct fastrpc_buf *imem = NULL;
> + int err = 0;
> +
> + ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
> + if (!ioctl)
> + return -ENOMEM;
> +
> + if (init->flags == FASTRPC_INIT_ATTACH) {
> + remote_arg_t ra[1];
> + int tgid = fl->tgid;
> +
> + ra[0].buf.pv = (void *)
> + ra[0].buf.len = sizeof(tgid);
> + ioctl->handle = 1;
> + ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> + ioctl->pra = ra;
> + fl->pd = 0;
> +
> + err = fastrpc_internal_invoke(fl, 1, ioctl);
> + if (err)
> + goto bail;
It doesn't seem right to me to dynamically allocate an 'ioctl' data structure
from kernel context and pass that down to another function. Maybe eliminate
that structure and change fastrpc_internal_invoke to take the individual
arguments here instead of a pointer?
> + } else if (init->flags == FASTRPC_INIT_CREATE) {
How about splitting each flags== case into a separate function?
> diff --git a/include/uapi/linux/fastrpc.h b/include/uapi/linux/fastrpc.h
> index 8fec66601337..6b596fc7ddf3 100644
> --- a/include/uapi/linux/fastrpc.h
> +++ b/include/uapi/linux/fastrpc.h
> @@ -6,6 +6,12 @@
> #include
>
> #define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_ioctl_invoke)
> +#define FASTRPC_IOCTL_INIT _IOWR('R', 4, struct fastrpc_ioctl_init)
> +
> +/* INIT a new process or attach to guestos */
> +#define FASTRPC_INIT_ATTACH 0
> +#define FASTRPC_INIT_CREATE 1
> +#define FASTRPC_INIT_CREATE_STATIC 2
Maybe use three command codes here, and remove the 'flags' member?
> @@ -53,4 +59,16 @@ struct fastrpc_ioctl_invoke {
> unsigned int *crc;
> };
>
> +struct fastrpc_ioctl_init {
> + uint32_t flags; /* one of FASTRPC_INIT_* macros */
> + uintptr_t file; /* pointer to elf file */
> + uint32_t filelen; /* elf file length */
> + int32_t filefd; /* ION fd for the file */
What does this have to do with ION? The driver seems to otherwise
just use the generic dma_buf interfaces.
> + uintptr_t mem; /* mem for the PD */
> + uint32_t memlen;/* mem length */
> + int32_t memfd; /* fd for the mem */
> + int attrs;
> + unsigned int siglen;
> +};
This structure is again not suitable for ioctls. Please used fixed-length
members and no holes in the structure.
Arnd
Re: [RFC PATCH 6/6] char: fastrpc: Add support for compat ioctls
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla wrote: > > From: Thierry Escande > > This patch adds support for compat ioctl from 32 bits userland to > Qualcomm fastrpc driver. > > Supported ioctls in this change are INIT, INVOKE, and ALLOC/FREE_DMA. > > Most of the work is derived from various downstream Qualcomm kernels. > Credits to various Qualcomm authors who have contributed to this code. > Specially Tharun Kumar Merugu > > Signed-off-by: Thierry Escande > Signed-off-by: Srinivas Kandagatla Can't you just define the native ioctls so that you don't need this. Arnd
Re: [PATCH v2] signal: add procfd_signal() syscall
On Fri, Nov 30, 2018 at 7:56 AM Christian Brauner wrote:
> On Thu, Nov 29, 2018 at 11:13:57PM -0600, Eric W. Biederman wrote:
> > Arnd Bergmann writes:
> > > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski
> > > wrote:
> > >
> > > It looks like we already have a 'struct signalfd_siginfo' that is defined
> > > in a
> > > sane architecture-independent way, so I'd suggest we use that.
> >
> > Unfortunately it isn't maintained very well. What you can
> > express with signalfd_siginfo is a subset what you can express with
> > siginfo. Many of the linux extensions to siginfo for exception
> > information add pointers and have integers right after those pointers.
> > Not all of those linux specific extentions for exceptions are handled
> > by signalfd_siginfo (it needs new fields).
Would those fit in the 30 remaining padding bytes?
> > As originally defined siginfo had the sigval_t union at the end so it
> > was perfectly fine on both 32bit and 64bit as it only had a single
> > pointer in the structure and the other fields were 32bits in size.
The problem with sigval_t of course is that it is incompatible between
32-bit and 64-bit. We can add the same information, but at least on
the syscall level that would have to be a __u64.
> > Although I do feel the pain as x86_64 has to deal with 3 versions
> > of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
> > with a 64bit si_clock_t.
At least you and Al have managed to get it down to a single source-level
definition across all architectures, but there is also the (lesser) problem
that the structure has a slightly different binary layout on each of the
classic architectures.
If we go with Andy's suggestion of having only a single binary layout
on x86 for the new call, I'd argue that we should also make it have
the exact same layout on all other architectures.
> > > We may then also want to make sure that any system call that takes a
> > > siginfo has a replacement that takes a signalfd_siginfo, and that this
> > > replacement can be used to implement the old version purely in
> > > user space.
> >
> > If you are not implementing CRIU and reinserting exceptions to yourself.
> > At most user space wants the ability to implement sigqueue:
> >
> > AKA:
> > sigqueue(pid_t pid, int sig, const union sigval value);
> >
> > Well sigqueue with it's own si_codes so the following would cover all
> > the use cases I know of:
> > int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
> >
> > The si_code could even be set to SI_USER to request that the normal
> > trusted SI_USER values be filled in. si_code values of < 0 if not
> > recognized could reasonably safely be treated as the _rt member of
> > the siginfo union. Or perhaps better we error out in such a case.
> >
> > If we want to be flexible and not have N kinds of system calls that
> > is the direction I would go. That is simple, and you don't need any of
> > the rest.
I'm not sure I understand what you are suggesting here. Would the
low-level implementation of this be based on procfd, or do you
mean that would be done for pid_t at the kernel level, plus another
syscall for procfd?
> > Alternatively we abandon the mistake of sigqueueinfo and not allow
> > a signal number in the arguments that differs from the si_signo in the
> > siginfo and allow passing the entire thing unchanged from sender to
> > receiver. That is maximum flexibility.
This would be regardless of which flavor of siginfo (today's arch
specific one, signalfd_siginfo, or a new one) we pass, right?
> > signalfd_siginfo just sucks in practice. It is larger that siginfo 104
> > bytes versus 48. We must deliver it to userspace as a siginfo so it
> > must be translated. Because of the translation signalfd_siginfo adds
> > no flexiblity in practice, because it can not just be passed through.
> > Finallay signalfd_siginfo does not have encodings for all of the
> > siginfo union members, so it fails to be fully general.
> >
> > Personally if I was to define signalfd_siginfo today I would make it:
> > struct siginfo_subset {
> > __u32 sis_signo;
> > __s32 sis_errno;
> > __s32 sis_code;
> > __u32 sis_pad;
> > __u32 sis_pid;
> > __u32 sis_uid;
> > __u64 sis_data (A pointer or integer data field);
> > };
> >
> > That is just 32bytes, and is all that is needed for everything
> > except for synchronous exceptions. Oh and that happens to be a proper
> > subset of a any sane siginfo layout, on both 32bit and 64bit.
And that would
Re: [PATCH v2] signal: add procfd_signal() syscall
On Thu, Nov 29, 2018 at 10:35 PM Christian Brauner wrote: > On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote: > > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski wrote: > > > > Is the current procfd_signal() proposal (under whichever name) sufficient > > to correctly implement both sys_rt_sigqueueinfo() and > > sys_rt_tgsigqueueinfo()? > > Yes, I see no reason why not. My idea is to extend it - after we have a > basic version in - to also work with: > /proc//task/ > If I'm not mistaken this should be sufficient to get rt_tgsigqueueinfo. > The thread will be uniquely identified by the tid descriptor and no > combination of /proc/ and /proc//task/ is needed. Does > that sound reasonable? Yes. So it would currently replace rt_gsigqueueinfo() but not rt_tgsigqueueinfo(), and could be extended to do both afterwards, without making the interface ugly in any form? I suppose we can always add more flags if needed, and you already ensure that flags is zero for the moment. > > Can we implement sys_rt_sigtimedwait() based on signalfd()? > > > > If yes, that would leave waitid(), which already needs a replacement > > for y2038, and that should then also return a signalfd_siginfo. > > My current preference for waitid() would be to do a version that > > closely resembles the current interface, but takes a signalfd_siginfo > > and a __kernel_timespec based rusage replacement (possibly > > two of them to let us map wait6), but does not operate on procfd or > > take a signal mask. That would require yet another syscall, but I > > don't think I can do that before we want to have the set of y2038 > > safe syscalls. > > All sounds reasonable to me but that's not a blocker for the current > syscall though, is it? I'd like to at least understand about sys_rt_sigtimedwait() before we go on, so we all know what's coming, and document the plans in the changelog. waitid() probably remains on my plate anyway, and I hope understand where we're at with it. Arnd
Re: [PATCH v2] signal: add procfd_signal() syscall
On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski wrote: > > On Nov 29, 2018, at 11:55 AM, Christian Brauner > > wrote: > >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote: > >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner > >>> wrote: > On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski > wrote: > >> > >> The #1 fix would add a copy_siginfo_from_user64() or similar. > > > > Thanks very much! That all helped a bunch already! I'll try to go the > > copy_siginfo_from_user64() way first and see if I can make this work. If > > we do this I would however only want to use it for the new syscall first > > and not change all other signal syscalls over to it too. I'd rather keep > > this patchset focussed and small and do such conversions caused by the > > new approach later. Does that sound reasonable? > > Absolutely. I don’t think we can change old syscalls — the ABI is set in > stone. > But for new syscalls, I think the always-64-bit behavior makes sense. It looks like we already have a 'struct signalfd_siginfo' that is defined in a sane architecture-independent way, so I'd suggest we use that. We may then also want to make sure that any system call that takes a siginfo has a replacement that takes a signalfd_siginfo, and that this replacement can be used to implement the old version purely in user space. Is the current procfd_signal() proposal (under whichever name) sufficient to correctly implement both sys_rt_sigqueueinfo() and sys_rt_tgsigqueueinfo()? Can we implement sys_rt_sigtimedwait() based on signalfd()? If yes, that would leave waitid(), which already needs a replacement for y2038, and that should then also return a signalfd_siginfo. My current preference for waitid() would be to do a version that closely resembles the current interface, but takes a signalfd_siginfo and a __kernel_timespec based rusage replacement (possibly two of them to let us map wait6), but does not operate on procfd or take a signal mask. That would require yet another syscall, but I don't think I can do that before we want to have the set of y2038 safe syscalls. Arnd
Re: [PATCH v3 6/6] mips: generate uapi header and system call table files
On Thu, Nov 29, 2018 at 9:45 AM Firoz Khan wrote: > diff --git a/arch/mips/include/uapi/asm/Kbuild > b/arch/mips/include/uapi/asm/Kbuild > index 7a4becd..ed4bd03 100644 > --- a/arch/mips/include/uapi/asm/Kbuild > +++ b/arch/mips/include/uapi/asm/Kbuild > @@ -1,5 +1,11 @@ > # UAPI Header export list > include include/uapi/asm-generic/Kbuild.asm > > +generated-y += unistd_n32.h > +generated-y += unistd_n64.h > +generated-y += unistd_o32.h > +generated-y += unistd_nr_n32.h > +generated-y += unistd_nr_n64.h > +generated-y += unistd_nr_o32.h > generic-y += bpf_perf_event.h > generic-y += ipcbuf.h I'd argue that the unistd_nr_*.h headers should not be in the uapi directory but instead be included only from the in-kernel header. > diff --git a/arch/mips/kernel/scall64-n64.S b/arch/mips/kernel/scall64-n64.S > new file mode 100644 > index 000..402a085 > --- /dev/null > +++ b/arch/mips/kernel/scall64-n64.S > @@ -0,0 +1,117 @@ > +/* > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details. > + * > + * Copyright (C) 1995, 96, 97, 98, 99, 2000, 01, 02 by Ralf Baechle > + * Copyright (C) 1999, 2000 Silicon Graphics, Inc. > + * Copyright (C) 2001 MIPS Technologies, Inc. > + */ > +#include > +#include > +#include > +#include It looks like you change and rename this file at the same time. Generally speaking, I would not do that in one patch. Either leave the slightly inconsistent name unchanged, or rename it in a separate patch. Arnd
Re: [PATCH 2/2] kernel/trace: fix watchdog soft lockup
On Wed, Nov 28, 2018 at 3:09 PM Steven Rostedt wrote:
>
> On Wed, 28 Nov 2018 09:13:34 +0100
> Anders Roxell wrote:
>
> > When building a allmodconfig kernel for arm64 and boot that in qemu,
> > CONFIG_FTRACE_STARTUP_TEST gets enabled and that takes time so the
> > watchdog expires and prints out a message like this:
> > 'watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [swapper/0:1]'
> > Each time the function ftrace_replace_code gets called it stays in that
> > functions loop for 41424 times.
> > Rework so that function cond_resched() gets called in the
> > ftrace_replace_code loop.
> >
> > Co-developed-by: Arnd Bergmann
> > Signed-off-by: Arnd Bergmann
> > Signed-off-by: Anders Roxell
> > ---
> > kernel/trace/ftrace.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 5b4f73e4fd56..3f456921dedf 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -2426,6 +2426,10 @@ void __weak ftrace_replace_code(int enable)
> >
> > do_for_each_ftrace_rec(pg, rec) {
> >
> > + /* This loop can take minutes when sanitizers are enabled, so
> > + * lets make sure we allow RCU processing.
> > + */
> > + cond_resched();
> > if (rec->flags & FTRACE_FL_DISABLED)
> > continue;
> >
>
> NACK. On some architectures this code is run from stop machine. We
> can't call cond_resched() because it may be called with interrupts
> disabled.
>
> This is a weak function. If arm64 has special needs, just copy it in
> the arm64 code.
I think it's currently broken on all architectures that don't already
override it, the problem being that the function is simply too
expensive when all debug options are enabled.
In an ARM64 allmodconfig kernel, there are 41424 records
that we iterate through several times. In an earlier version of the
test, the cond_resched() was only in the loop in
init_trace_selftests(), and I think that is safe and should /mostly/
solve the problem, so maybe Anders can submit that version again.
However, at least trace_selftest_ops() still takes half a minute
to complete in qemu, and that triggers the softlockup watchdog.
trace_selftest_ops() calls ftrace_replace_code() four or five times.
Here is the excerpt with printk times from one of Anders' tests:
[8.350607] Running postponed tracer tests:
[8.356045] Testing tracer function:
[ 18.932077] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 27.454205] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 27.462594] PASSED
[ 27.462954] Testing dynamic ftrace:
[ 28.510903] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 28.746934] PASSED
[ 28.747469] Testing dynamic ftrace ops #1:
[ 32.488427] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 32.501864] (1 0 1 0 0)
[ 32.502041] (1 1 2 0 0)
[ 50.213914] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 50.219736] (2 1 3 0 1066085)
[ 50.220077] (2 2 4 0 1066100)
[ 60.580678] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 60.758019] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 60.910501] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 60.918354] PASSED
[ 60.919672] Testing dynamic ftrace ops #2:
[ 64.680222] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 64.843430] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 81.247068] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 81.250895] (1 0 1 1033119 0)
[ 81.251186] (1 1 2 1033134 0)
[ 81.343168] (2 1 3 1 3732)
[ 81.344492] (2 2 4 118 3849)
[ 89.837665] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 89.844371] PASSED
[ 89.844719] Testing ftrace recursion:
[ 90.890373] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 91.042146] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 91.048475] PASSED
[ 91.048806] Testing ftrace recursion safe:
[ 92.091174] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 92.242403] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 92.249119] PASSED
[ 92.249470] Testing ftrace regs(no arch support):
[ 93.293605] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 93.444942] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 93.451738] PASSED
[ 93.452300] Testing tracer nop: PASSED
[ 93.453288] Testing tracer irqsoff:
[ 104.486368] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 112.918828] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 112.925809] PASSED
[ 112.926435] Testing tracer function_graph:
[ 123.303248] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 132.599763] ../kernel/trace/ftrace.c:2441, loop_counter: 41424
[ 132.607614] PASSED
In particula
Re: [PATCH v4 0/20] MMP platform fixes
On Wed, Nov 28, 2018 at 6:53 PM Lubomir Rintel wrote: > > Hi, > > this series contains contains a bunch of MMP2 platform fixes. > > Previous spins of the patch set were sent out over the course of last > three months to the MMP platform maintainers, with Arnd and > linux-arm-ker...@lists.infradead.org on copy. > > Unfortunatelly, MMP maintainers (Eric Miao and Haojian Zhuang) don't > seem to respond anymore. That's a shame, because the MMP2 support seems to > in need for some love. The DT/multiplatform kernels can't even boot without > 14/20, 15/20 and perhaps more. > > I'm wondering if this cat gen in via the arm-soc tree? Would it be > appropriate if I followed up with a MAINTAINERS update in that case? Yes, sounds good to me. I looked through the series again, and found one patch that seems wrong to me. Everything else should just go in. Patches that are purely bugfixes should probably also get backported to stable kernels, so please add a 'Cc: sta...@vger.kernel.org' tag in the changelog for things that are required to make older kernels work correctly (as opposed to adding features that were never supposed to work), and describe in the changelog which kernel versions should see that backport. > The patch set has been tested on an OLPC XO-1.75 laptop. Excellent! Arnd
Re: [PATCH v4 14/20] ARM: mmp/mmp2: use cpu_is_pj4() instead of cpu_is_mmp2()
On Wed, Nov 28, 2018 at 6:54 PM Lubomir Rintel wrote:
>
> The MMP2 platform uses the PJ4 CPU. The cpu_is_mmp2() macro is thus
> actually not useful at all and moreover gives the wrong result on
> MACH_MMP2_DT.
>
> The actual problem I aim to fix is that on a device-tree enabled system,
> the timer ends up being initialized incorrectly. In fact, it ticks like
> at rate that's 1/100 slower or so.
>
> Perhaps the other cpu_is_mmp2() uses are more benign, but still useless.
I think we still need the helper. If it's broken on some platforms,
it has to be fixed instead.
> @@ -104,7 +104,7 @@ void __init mmp2_init_irq(void)
>
> static int __init mmp2_init(void)
> {
> - if (cpu_is_mmp2()) {
> + if (cpu_is_pj4()) {
> #ifdef CONFIG_CACHE_TAUROS2
> tauros2_init(0);
> #endif
> diff --git a/arch/arm/mach-mmp/pm-mmp2.c b/arch/arm/mach-mmp/pm-mmp2.c
> index 17699be3bc3d..bcd5111ffb37 100644
> --- a/arch/arm/mach-mmp/pm-mmp2.c
> +++ b/arch/arm/mach-mmp/pm-mmp2.c
> @@ -220,7 +220,7 @@ static int __init mmp2_pm_init(void)
> {
> uint32_t apcr;
>
> - if (!cpu_is_mmp2())
> + if (!cpu_is_pj4())
> return -EIO;
>
> suspend_set_ops(_pm_ops);
These are both regular initcalls. On a multiplatform kernel, they
will be called unconditionally, and the cpu_is_mmp2() has to
guard code that would be wrong on other SoCs, including those
with a pj4 CPU (Armada XP, Dove, Berlin).
> diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c
> index 96ad1db0b04b..0f49ac579a17 100644
> --- a/arch/arm/mach-mmp/time.c
> +++ b/arch/arm/mach-mmp/time.c
> @@ -163,7 +163,7 @@ static void __init timer_config(void)
>
> __raw_writel(0x0, mmp_timer_base + TMR_CER); /* disable */
>
> - ccr &= (cpu_is_mmp2()) ? (TMR_CCR_CS_0(0) | TMR_CCR_CS_1(0)) :
> + ccr &= (cpu_is_pj4()) ? (TMR_CCR_CS_0(0) | TMR_CCR_CS_1(0)) :
> (TMR_CCR_CS_0(3) | TMR_CCR_CS_1(3));
> __raw_writel(ccr, mmp_timer_base + TMR_CCR);
>
This looks correct, here we just need to ensure we are not on pj1.
Note: we should probably rename 'timer_init' to something that
that has 'mmp' in the name, the current name conflicts with
a function of the same name in mach-davinci, which is not yet
multiplatform, but hopefully will be one day.
Arnd
Re: [PATCH v2] signal: add procfd_signal() syscall
On Tue, Nov 20, 2018 at 11:54 AM Christian Brauner wrote: > --- > arch/x86/entry/syscalls/syscall_32.tbl | 1 + > arch/x86/entry/syscalls/syscall_64.tbl | 2 + > fs/proc/base.c | 11 ++- > fs/proc/internal.h | 5 - > include/linux/proc_fs.h| 12 +++ > include/linux/syscalls.h | 2 + > include/uapi/asm-generic/unistd.h | 4 +- > kernel/signal.c| 127 +++-- > 8 files changed, 151 insertions(+), 13 deletions(-) For asm-generic: Acked-by: Arnd Bergmann I checked that the system call wired up correctly in a way that works on all architectures using the generic syscall table.
Re: [PATCH v2 1/3] ARM: imx_v6_v7_defconfig: Remove explicit ARM_UNWIND disable
On Sun, Nov 25, 2018 at 10:45 PM Otavio Salvador wrote: > > On Sun, Nov 25, 2018 at 7:35 PM Arnd Bergmann wrote: > > On Sun, Nov 25, 2018 at 10:24 PM Otavio Salvador > > wrote: > > > > > > CONFIG_ARM_UNWIND is removed when running 'savedefconfig', but > > > selected by the Kconfig logic. This is done in preparation to making > > > further changes to this defconfig cleaner. > > > > > > > Does this mean we no longer get the unwinder, or there is some other > > config (which?) that unconditionally selects it? > > It is selected. Before changing the defconfig I did a savedefconfig to > avoid unrelated changes to be included on the subsequent patches and > then I found it was now selected. Ok, then please mention in the changelog which other option selects it, as I said. Aside from that, your series looks fine to me. Arnd
Re: [PATCH v2 1/3] ARM: imx_v6_v7_defconfig: Remove explicit ARM_UNWIND disable
On Sun, Nov 25, 2018 at 10:24 PM Otavio Salvador wrote: > > CONFIG_ARM_UNWIND is removed when running 'savedefconfig', but > selected by the Kconfig logic. This is done in preparation to making > further changes to this defconfig cleaner. > Does this mean we no longer get the unwinder, or there is some other config (which?) that unconditionally selects it? Arnd
Re: [PATCH 1/6] char: lp: remove trailing whitespace
On Sun, Nov 25, 2018 at 8:47 PM Sudip Mukherjee wrote: > > Fix checkpatch error for trailing whitespace. > > Signed-off-by: Sudip Mukherjee All six patches Acked-by: Arnd Bergmann They only touch whitespace and related coding style, so that's all fine.
Re: [PATCH 11/17] soc: ti: pruss: add pruss_get()/put() API
On Thu, Nov 22, 2018 at 12:41 PM Roger Quadros wrote: > + > + if (IS_ERR_OR_NULL(rproc)) > + return ERR_PTR(-EINVAL); Any usage of IS_ERR_OR_NULL() tends to be an indication of a badly designed API. Please change this to allow only one of the two to be passed around. Arnd
Re: [PATCH] soc/tegra: refactor soc_is_tegra()
On Thu, Nov 22, 2018 at 11:45 AM Arnd Bergmann wrote:
>
> On Wed, Nov 21, 2018 at 3:40 PM Yangtao Li wrote:
> >
> > of_find_node_by_path() acquires a reference to the node returned by
> > it and that reference needs to be dropped by its caller.soc_is_tegra()
> > doesn't do that, so fix it.Call of_machine_is_compatible() to refactor
> > soc_is_tegra() whcih automatically manages the reference count.
> >
> > Signed-off-by: Yangtao Li
> > ---
Anders ran into a crash after this patch, on a non-tegra qemu platform:

[0.055907] ASID allocator initialised with 32768 entries
[0.063381] rcu: Hierarchical SRCU implementation.
[0.137238] Unable to handle kernel paging request at virtual
address 0935f0c0
[0.137274] Mem abort info:
[0.137291] ESR = 0x9607
[0.137320] Exception class = DABT (current EL), IL = 32 bits
[0.137337] SET = 0, FnV = 0
[0.137352] EA = 0, S1PTW = 0
[0.137370] Data abort info:
[0.137387] ISV = 0, ISS = 0x0007
[0.137401] CM = 0, WnR = 0
[0.137456] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (ptrval)
[0.137479] [0935f0c0] pgd=bdfff003,
pud=bdffe003, pmd=bdffa003, pte=
[0.137644] Internal error: Oops: 9607 [#1] PREEMPT SMP
[0.137766] Modules linked in:
[0.137950] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.20.0-rc3-next-20181122-6-g38d8a1f80349-dirty #2
[0.137975] Hardware name: linux,dummy-virt (DT)
[0.138071] pstate: 0085 (nzcv daIf -PAN -UAO)
[0.138110] pc : __of_device_is_compatible+0x30/0x138
[0.138132] lr : of_device_is_compatible+0x40/0x68
[0.138147] sp : 0804bc80
[0.138167] x29: 0804bc80 x28:
[0.138207] x27: x26:
[0.138224] x25: 80007dfed2a8 x24: 09301000
[0.138239] x23: x22:
[0.138254] x21: 0935f0c0 x20: 0935f0c0
[0.138269] x19: x18: 0400
[0.138284] x17: x16:
[0.138298] x15: 0400 x14: 0400
[0.138313] x13: x12: 0020
[0.138327] x11: 0008 x10: 0101010101010101
[0.138357] x9 : 6862726efffefeff x8 : 7f7f7f7f7f7f7f7f
[0.138373] x7 : fefefefefefeff2e x6 : 0080808080808080
[0.138387] x5 : 0002 x4 : 0001
[0.138402] x3 : x2 :
[0.138416] x1 : 0935f0c0 x0 : 80007dfed2a8
[0.138475] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[0.138540] Call trace:
[0.138607] __of_device_is_compatible+0x30/0x138
[0.138632] of_device_is_compatible+0x40/0x68
[0.138654] of_machine_is_compatible+0x34/0x68
[0.138672] soc_is_tegra+0x2c/0x40
[0.138689] tegra_flowctrl_init+0x28/0x108
[0.138706] do_one_initcall+0x5c/0x178
[0.138722] kernel_init_freeable+0xd0/0x240
[0.138741] kernel_init+0x10/0x108
[0.138755] ret_from_fork+0x10/0x18
[0.138913] Code: b4000861 f90013b5 aa0103f5 52800013 (39400021)
[0.139229] ---[ end trace d4d0fc77e9b04fa6 ]---
[0.139448] note: swapper/0[1] exited with preempt_count 1
[0.140598] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x000b
[0.140767] ---[ end Kernel panic - not syncing: Attempted to kill
init! exitcode=0x000b ]---
I'm not completely sure what's wrong with the patch, but I assume
it never worked on non-tegra machines.
> > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> > index cd8f41351add..0b40700b672a 100644
> > --- a/drivers/soc/tegra/common.c
> > +++ b/drivers/soc/tegra/common.c
> > @@ -22,11 +22,13 @@ static const struct of_device_id tegra_machine_match[]
> > = {
> >
> > bool soc_is_tegra(void)
> > {
> > - struct device_node *root;
> > + struct of_device_id *match = tegra_machine_match;
> >
> > - root = of_find_node_by_path("/");
> > - if (!root)
> > - return false;
> > + while(match->compatible){
> > + if(of_machine_is_compatible(match->compatible))
> > + return true;
> > + match++;
> > + }
> >
> > - return of_match_node(tegra_machine_match, root) != NULL;
> > + return false;
I would also note that this is a rather inefficient way to check
for a particular platform, as we to do the string search through
the DT for each entry in the table now instead of doing it once.
Arnd
Re: [PATCH] soc/tegra: refactor soc_is_tegra()
On Wed, Nov 21, 2018 at 3:40 PM Yangtao Li wrote:
>
> of_find_node_by_path() acquires a reference to the node returned by
> it and that reference needs to be dropped by its caller.soc_is_tegra()
> doesn't do that, so fix it.Call of_machine_is_compatible() to refactor
> soc_is_tegra() whcih automatically manages the reference count.
>
> Signed-off-by: Yangtao Li
> ---
> drivers/soc/tegra/common.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index cd8f41351add..0b40700b672a 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -22,11 +22,13 @@ static const struct of_device_id tegra_machine_match[] = {
>
> bool soc_is_tegra(void)
> {
> - struct device_node *root;
> + struct of_device_id *match = tegra_machine_match;
>
> - root = of_find_node_by_path("/");
> - if (!root)
> - return false;
> + while(match->compatible){
> + if(of_machine_is_compatible(match->compatible))
> + return true;
> + match++;
> + }
>
> - return of_match_node(tegra_machine_match, root) != NULL;
> + return false;
> }
> --
> 2.17.0
>
Re: Cleaning up numbering for new x86 syscalls?
On Tue, Nov 20, 2018 at 4:35 PM Andy Lutomirski wrote: > > On Tue, Nov 20, 2018 at 1:03 AM Florian Weimer wrote: > > > > * Andy Lutomirski: > > > > > 5. Adjust the scripts so that we only have to wire up new syscalls > > > once. They'll have a nr above 1024, and they'll have the same nr on > > > all x86 variants. > > > > Is there a sufficiently sized gap on all other architectures as well? > > The restriction to the x86 variants seems arbitrary to me. > > > > Fair point. We have this shiny "generic" syscall list. Maybe we can > get x86 synced up with it for new syscalls. The generic table is already a subset of the x86 tables, so there should be no need to sync up the contents. It's more critical on other architectures that currently lack a number of the syscalls that got added in asm-generic and x86 recently, so I'd like to synchronize these all and add the missing calls to ensure that each architecture has at least all the calls from asm-generic table. After that, I would hope to come up with a way to add future numbers to all tables together, either using the same numbers everywhere (plus an offset where necessary, e.g. mips), or even have an include file logic so we only need a single file for future additions. Note: for y2038, we will have to add around 20 to 25 syscalls to each 32-bit architecture, plus another 10 for those that lack the separate sys_ipc calls. Arnd
Re: Cleaning up numbering for new x86 syscalls?
On Tue, Nov 20, 2018 at 1:25 AM Andy Lutomirski wrote: > > Hi all- > > We currently have some giant turds in the way that syscalls are > numbered. We have the x86_32 table, which is totally sane other than > some legacy multiplexers. Then we have the x86_64 table, which is, > um, demented: > > - The numbers don't match x86_32. I have no idea why. I think it was an early attempt at cleanup up the table, and only adding those that were still used. Back in the days, each architecture had its own table, and of course they started out as separate top-level architectures. > - We use bit 30, which triggers in_x32_syscall(). It should have > been bit 31, bit I digress. > > - We have this weird set of extra x32 syscalls that start at 512. > Who wants to bet whether we have no bugs if someone does syscall with, > say, nr == 512 (i.e. not 512 | BIT(30)) or nr == (16 | BIT(30))? The > latter would be non-compat ioctl with in_x32_syscall() set and hence > in_compat_syscall() set. The comment in the table says it's purely for keeping the calls in separate cache lines. I don't know if the cache lines make a difference in the end, but it seems that once we start running into the x32 syscall numbers, I think we just treat them like any others, we just choose to never call them from a 64-bit glibc. > I propose we consider some subset of the following: > > 1. Introduce restart_syscall_2(). Make its number be 1024. Maybe > someday we could start using it instead of restart_syscall(). The > only issue I can see is programs that allow restart_syscall() using > seccomp but don't allow the new variant. > > 2. Introduce an outright ban on new syscalls with nr < 1024. This would leave a hole of several hundred numbers if we do it for all architectures. Wasting multiple kilobytes for a cosmetic cleanup might be considered excessive. > 3. Introduce an outright ban on the addition of new __x32_compat > syscalls. If new compat hacks are needed, they can use > in_compat_syscall(), thank you very much. I would definitely want to keep anything regarding x32 out of the common syscall implementation. If you want to add on to that pile, please do it in arch/x86, not in kernel/ or fs/. If we decide that x32 is a failed experiment and we don't keep it working in the future, let's just kill it off right away. I'm fairly sure nobody depends on it for anything real, the only users I could find are either for showing off benchmark results or for playing around with it for fun. Most of that fun part has apparently ended many years ago, but there is still some work going into debian/x32. We probably need to coordinate with them and see if they know of actual users before removing it. Popcon lists 5 active users [1] and a sharp downward trend. > 4. Modify the wrappers of the __x32_compat entries so that they will > return -ENOSYS if in_x32_syscall() returns false. No objection here, but what would that help? > 5. Adjust the scripts so that we only have to wire up new syscalls > once. They'll have a nr above 1024, and they'll have the same nr on > all x86 variants. > > Thoughts? I would definitely welcome assigning the same syscall numbers across all architectures. It is a needless burden for the libc developers to figure out for each syscall which kernel is known to support it. When a call gets added, they typically add logic to check for the system call at runtime, but for older syscalls, it helps to know when all architectures support it once the minimum kernel version for a libc has been raised beyond that. Please see also the work that Firoz Khan has been posting for generalizing the tables on all architectures to use the format we have on x86, arm and s390. I hope we can merge it all for 4.21, and then build on top of that for generalization and cleanups. Arnd [1] https://popcon.debian.org/stat/sub-x32.png
Re: [PATCH v2 06/15] arm: dts: Add devicetree for OrangePi 2G IoT board
On Wed, Nov 21, 2018 at 4:38 AM Manivannan Sadhasivam
wrote:
> + aliases {
> + serial0 =
> + serial1 =
> + serial2 =
> + };
>
+
> + {
> + status = "okay";
> + clocks = <_clk>;
> +};
This is clearly mismatched here: you mark only one uart as 'enabled, but
list three of them as aliases. Having 'serial0' point to a disabled uart
may easily break applications that expect the first one to be the
console.
Best make that
serial0 =
and drop the other ones if only one of them is exposed on the
board. If all three are usable, you should enable them all here,
and make sure that the numbering of the aliases matches the
labels on the board or its documentation.
Arnd
Re: [PATCH 0/8] BCM2835 PM driver
On Tue, Nov 20, 2018 at 10:34 PM Eric Anholt wrote: > >> Eric Anholt hat am 20. November 2018 um 18:19 > >> geschrieben: > >> > >> > >> This series moves the BCM2835 WDT driver that controls a fraction of > >> the PM block out to soc/ and adds most of the rest of its > >> functionality. My motivation has been to have V3D be functional > >> without firmware calls, probably improve its interactivity (since > >> we'll be able to power on/off without RPC to the firmware that may be > >> busy with other tasks), and (in a patch not submitted in this series) > >> extend its binding to use the reset controller instead of trying to > >> reset by toggling its power domain. > >> > >> I've tested V3D with a few hours of running a V3D test, sleep(1) (to > >> trigger PM domain off); running a GPU hang job (to trigger reset); > >> sleep(1). The non-hanging success-case job always passed, and dmesg > >> had no complaints from bcm2835-pm. The other power domains are not > >> tested, but I've done my best. > >> > >> This series will probably also be of interest to the > >> https://github.com/christinaa/rpi-open-firmware project for enabling USB. > >> > > > > apologize to give you my feedback after you send out the series. > > > > I know you won't be happy about it, but i think we need a little more > > complex but future proof solution for this power driver. According to > > the register definition of the PM block, we have multiple functions > > here (power domains, watchdog, pads/pinctrl, ...). Since this is > > common for ARM SoCs there is a subsystem called mfd (multi function > > device) [1] to abstract all resources of the IP block. > > > > This has the advantage that we don't need a monolithic driver which > > takes care of all functions. > > I consider your "advantage" to be a disadvantage. By forcing the split, > you end up having more driver files to manage, more platform devices, > and more error-prone code to get the resources from the parent down into > the client. It feels like writing software for the sake of writing > software, rather than solving a concrete problem. > > My original series: > > 10 files changed, 951 insertions(+), 273 deletions(-) > > The MFD series: > > 12 files changed, 882 insertions(+), 25 deletions(-) My preference in general is having less code in drivers/soc and more in subsystems of people that understand their stuff. Not every driver leans itself to being an MFD, but the more differnent functions it has, the better it is to have it split up. This was the original motivation of moving irqchip, clk, pinctrl, etc drivers out of arch/arm/mach-* into their own subsystems. Arnd
Re: BUG: unable to handle kernel NULL pointer dereference in write_port
On Tue, Nov 13, 2018 at 9:24 AM Kyungtae Kim wrote: > > We report a bug in v4.19-rc8 (4.20-rc1 as well): > > kernel config: https://kt0755.github.io/etc/config-4.19-rc2.kmsan > repro: https://kt0755.github.io/etc/repro.e3752.c > > This happens during data transition from user-supplied buffer to port > (using outb) pointed by ppos. (driver/mem/char.c:640) > Although there is a strict bound 65536 (driver/mem/char.c:632), user > buffer copy still causes crashes within the strict bound. > (In the experiment, the crash arose when loop count is beyond 0x7f ) > To stop it, it probably needs a little tight bound check. > > I think this is a little bit related to the crash I reported before > (https://lkml.org/lkml/2018/5/12/91) > > Crash log > = > BUG: unable to handle kernel NULL pointer dereference at 00af The first thing that comes to mind is that this would be qemu specific. Note that writing arbitrary data into arbitrary I/O ports is likely to crash any x86 PC, but it's possible that qemu reports a different set of exceptions. Looking in /proc/ioports for a real PC, I find 0080-008f : dma page reg 00a0-00a1 : pic2 00c0-00df : dma2 00f0-00ff : fpu so it appears that you have just written into the interrupt controller here. Arnd
Re: [PATCH 04/16] arm: dts: Add devicetree for RDA8810PL SoC
On Mon, Nov 19, 2018 at 6:11 PM Manivannan Sadhasivam
wrote:
>
> Add initial device tree for RDA8810PL SoC from RDA Microelectronics.
> + aliases {
> + serial0 =
> + serial1 =
> + serial2 =
> + };
Better move the aliases into the board file, there might be boards
that only expose one or two of them on physical connectors,
or that count them in a different order from the SoC.
Arnd
Re: [PATCH v2 3/4] powerpc: add system call table generation support
On Wed, Nov 14, 2018 at 11:04 AM Firoz Khan wrote: > Adding a new table entry consisting of: > - System call number. > - ABI. > - System call name. > - Entry point name. > - Compat entry name, if required. > > syscallhdr.sh and syscalltbl.sh will generate uapi header- > unistd_32/64.h and syscall_table_32/64/c32.h files respect- > ively. File syscall_table_32/64/c32.h is included by sys- > call.S - the real system call table. Both .sh files will > parse the content syscall.tbl to generate the header and > table files. You don't mention how this handles the SPU, which seems to be the main difference from the other architectures. > +# The format is: > +# > +# > +# The can be common, 64, or 32 for this file. > +# > +0 common restart_syscall sys_restart_syscall > sys_restart_syscall > +1 common exitsys_exit > sys_exit > +2 common forkppc_fork > ppc_fork > +3 common readsys_read > sys_readsys_read > +4 common write sys_write > sys_write sys_write > +5 common opensys_open > compat_sys_open sys_open > +6 common close sys_close > sys_close sys_close > +7 common waitpid sys_waitpid > sys_waitpid sys_waitpid > +8 common creat sys_creat > sys_creat sys_creat The SPU syscall is always the same as the 64-bit syscall, so listing it explictily in the last column seems to add a lot of duplication, and makes the format different from the other architectures. Have you considered using the field (second column) to decide whether a syscall is used for SPU or not instead? I would have done it like |+0 nospu restart_syscall sys_restart_syscall sys_restart_syscall |+1 nospu exitsys_exit sys_exit |+2 nospu forkppc_fork ppc_fork |+3 common readsys_read sys_read |+4 common write sys_write sys_write |+5 common opensys_open compat_sys_open |+6 common close sys_close sys_close ... |+29132 fstatat64 sys_fstatat64 sys_fstatat64 |+29164 newfstatat sys_newfstatat |+291spu newfstatat sys_newfstatat ... with 'nospu' meaning 64+32+compat. > +9 common linksys_link > sys_linksys_link > +10 common unlink sys_unlink > sys_unlink sys_unlink > +11 common execve sys_execve > compat_sys_execve > +12 common chdir sys_chdir > sys_chdir sys_chdir > +13 common timesys_time > compat_sys_time sys_time > +14 common mknod sys_mknod > sys_mknod sys_mknod > +15 common chmod sys_chmod > sys_chmod sys_chmod > +16 common lchown sys_lchown > sys_lchown sys_lchown > +17 common break sys_ni_syscall > sys_ni_syscall > +18 32 oldstat sys_stat > sys_ni_syscall > +18 64 oldstat sys_ni_syscall 'oldstat' seems odd. Your conversion is correct, but the existing behavior of not providing support for the syscall in compat mode feels wrong. We have four calls in this category: arch/powerpc/include/asm/systbl.h:OLDSYS(stat) arch/powerpc/include/asm/systbl.h:OLDSYS(fstat) arch/powerpc/include/asm/systbl.h:OLDSYS(lstat) arch/powerpc/include/asm/systbl.h:OLDSYS(debug_setcontext) For the first three, it seems that the 64-bit kernel ought to set '__ARCH_WANT_OLD_STAT': diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h index 96ddce5c76c3..335dfcc47f20 100644 --- a/arch/powerpc/include/asm/unistd.h +++
Re: [PATCH v2 2/4] powerpc: move macro definition from asm/systbl.h
On Wed, Nov 14, 2018 at 11:04 AM Firoz Khan wrote: > diff --git a/arch/powerpc/include/asm/systbl.h > b/arch/powerpc/include/asm/systbl.h > index 01b5171..c4321b9 100644 > --- a/arch/powerpc/include/asm/systbl.h > +++ b/arch/powerpc/include/asm/systbl.h > @@ -76,7 +76,6 @@ > SYSCALL_SPU(ssetmask) > SYSCALL_SPU(setreuid) > SYSCALL_SPU(setregid) > -#define compat_sys_sigsuspend sys_sigsuspend > SYS32ONLY(sigsuspend) I think the macro here is just a workaround for the fact that SYS32ONLY() always prepends the name with 'compat_' for the compat version, and there is no other macro to do this. After the conversion, this can easily be done using the regular table, as you need separate names for the 32-bit entries anyway. > SYSX(sys_ni_syscall,compat_sys_s > diff --git a/arch/powerpc/platforms/cell/spu_callbacks.c > b/arch/powerpc/platforms/cell/spu_callbacks.c > index 8ae8620..7517a43 100644 > --- a/arch/powerpc/platforms/cell/spu_callbacks.c > +++ b/arch/powerpc/platforms/cell/spu_callbacks.c > @@ -47,6 +47,7 @@ > #define COMPAT_SPU_NEW(func) sys_##func, > #define SYSX_SPU(f, f3264, f32)f, > > +#define compat_sys_sigsuspend sys_sigsuspend > #include > }; The spu_callbacks.c and systbl_chk.c files don't need this macro, but that doesn't matter once you drop this patch. Arnd
Re: [PATCH v2 5/5] mips: generate uapi header and system call table files
On Thu, Nov 15, 2018 at 7:15 AM Firoz Khan wrote: > > System call table generation script must be run to gener- > ate unistd_64/n32/o32.h and syscall_table_32_o32/64_64/64- > _n32/64-o32.h files. This patch will have changes which > will invokes the script. > > This patch will generate unistd_64/n32/o32.h and syscall- > _table_32_o32/64_64/64-n32/64-o32.h files by the syscall > table generation script invoked by parisc/Makefile and > the generated files against the removed files must be > identical. > > The generated uapi header file will be included in uapi/- > asm/unistd.h and generated system call table header file > will be included by kernel/scall32-o32/64-64/64-n32/- > 64-o32.Sfile. > > Signed-off-by: Firoz Khan Looks good to me. Arnd
Re: [PATCH v2 4/5] mips: add system call table generation support
On Thu, Nov 15, 2018 at 7:15 AM Firoz Khan wrote: > > The system call tables are in different format in all > architecture and it will be difficult to manually add, > modify or delete the syscall table entries in the res- > pective files. To make it easy by keeping a script and > which will generate the uapi header and syscall table > file. This change will also help to unify the implemen- > tation across all architectures. This looks great to me, just one question: > +# The is always "64" for this file. > +# > +0 64 readsys_read > +1 64 write sys_write What is the reason for using '64', 'n32', and 'o32' respectively in the ABI field but use 'common' in other architectures that have a table of entries that are all for the same architecture? Arnd
Re: [PATCH v2 2/5] mips: add +1 to __NR_syscalls in uapi header
On Thu, Nov 15, 2018 at 7:15 AM Firoz Khan wrote:
>
> All other architectures are hold a value for __NR_syscalls will
> be equal to the last system call number +1.
>
> But in mips architecture, __NR_syscalls hold the value equal to
> total number of system exits in the architecture. One of the
> patch in this patch series will genarate uapi header files.
>
> In order to make the implementation common across all architect-
> ures, add +1 to __NR_syscalls, which will be equal to the last
> system call number +1.
>
> Signed-off-by: Firoz Khan
The patch looks correct to me, and is a nice cleanup, but I found a
couple of things remaining that could be done slightly better.
> diff --git a/arch/mips/include/asm/unistd.h b/arch/mips/include/asm/unistd.h
> index c68b8ae..16f21c3 100644
> --- a/arch/mips/include/asm/unistd.h
> +++ b/arch/mips/include/asm/unistd.h
> @@ -15,11 +15,11 @@
> #include
>
> #ifdef CONFIG_MIPS32_N32
> -#define NR_syscalls (__NR_N32_Linux + __NR_N32_Linux_syscalls)
> +#define NR_syscalls (__NR_N32_Linux + __NR_N32_Linux_syscalls - 1)
> #elif defined(CONFIG_64BIT)
> -#define NR_syscalls (__NR_64_Linux + __NR_64_Linux_syscalls)
> +#define NR_syscalls (__NR_64_Linux + __NR_64_Linux_syscalls - 1)
> #else
> -#define NR_syscalls (__NR_O32_Linux + __NR_O32_Linux_syscalls)
> +#define NR_syscalls (__NR_O32_Linux + __NR_O32_Linux_syscalls - 1)
> #endif
I suppose these can simply get removed, there are no users of NR_syscalls
in MIPS kernels.
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 7f3dfdb..add4301 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
> @@ -410,13 +410,13 @@ unsigned long __init arch_syscall_addr(int nr)
> unsigned long __init arch_syscall_addr(int nr)
> {
> #ifdef CONFIG_MIPS32_N32
> - if (nr >= __NR_N32_Linux && nr <= __NR_N32_Linux +
> __NR_N32_Linux_syscalls)
> + if (nr >= __NR_N32_Linux && nr <= __NR_N32_Linux +
> __NR_N32_Linux_syscalls - 1)
> return (unsigned long)sysn32_call_table[nr - __NR_N32_Linux];
> #endif
> - if (nr >= __NR_64_Linux && nr <= __NR_64_Linux +
> __NR_64_Linux_syscalls)
> + if (nr >= __NR_64_Linux && nr <= __NR_64_Linux +
> __NR_64_Linux_syscalls - 1)
> return (unsigned long)sys_call_table[nr - __NR_64_Linux];
> #ifdef CONFIG_MIPS32_O32
> - if (nr >= __NR_O32_Linux && nr <= __NR_O32_Linux +
> __NR_O32_Linux_syscalls)
> + if (nr >= __NR_O32_Linux && nr <= __NR_O32_Linux +
> __NR_O32_Linux_syscalls - 1)
> return (unsigned long)sys32_call_table[nr - __NR_O32_Linux];
> #endif
Here I would drop the '-1' and instead replace the '<=' with '<' for
better readability
> diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
> index 91d3c8c..a9b895f 100644
> --- a/arch/mips/kernel/scall32-o32.S
> +++ b/arch/mips/kernel/scall32-o32.S
> @@ -23,7 +23,7 @@
> #include
>
> /* Highest syscall used of any syscall flavour */
> -#define MAX_SYSCALL_NO __NR_O32_Linux + __NR_O32_Linux_syscalls
> +#define MAX_SYSCALL_NO __NR_O32_Linux + __NR_O32_Linux_syscalls - 1
This is also unused as of commit 2957c9e61ee9 ("[MIPS] IRIX: Goodbye and
thanks for all the fish"), eight years ago, so I'd remove this as well.
I'd suggest doing one patch for the removal of the unused macros, and another
patch for the other changes.
Arnd
Re: [PATCH v2 1/5] mips: add __NR_syscalls along with __NR_Linux_syscalls
On Thu, Nov 15, 2018 at 7:14 AM Firoz Khan wrote: > > The 2nd option will be the recommended one. For that, I > added the __NR_syscalls macro in uapi/asm/unistd.h along > with __NR_Linux_syscalls. The macro __NR_syscalls also > added for making the name convention same across all > architecture. While __NR_syscalls isn't strictly part of > the uapi, having it as part of the generated header to > simplifies the implementation. We also need to enclose > this macro with #ifdef __KERNEL__ to avoid side effects. I fear this doesn't work the way you hoped: > --- a/arch/mips/include/uapi/asm/unistd.h > +++ b/arch/mips/include/uapi/asm/unistd.h > @@ -391,16 +391,19 @@ > #define __NR_rseq (__NR_Linux + 367) > #define __NR_io_pgetevents (__NR_Linux + 368) > > +#ifdef __KERNEL__ > +#define __NR_syscalls 368 > +#endif We now have three different definitions of __NR_syscalls, one for each ABI. User space previously saw the correct one (now it doesn't see any, but that's ok). > /* > * Offset of the last Linux o32 flavoured syscall > */ > -#define __NR_Linux_syscalls368 > +#define __NR_Linux_syscalls__NR_syscalls so this part part again is ok. > #endif /* _MIPS_SIM == _MIPS_SIM_ABI32 */ > > #define __NR_O32_Linux 4000 > -#define __NR_O32_Linux_syscalls368 > +#define __NR_O32_Linux_syscalls__NR_syscalls but this part is not: Now __NR_O32_Linux_syscalls is defined to __NR_syscalls, which may be one of the three values. Any usage of __NR_O32_Linux_syscalls in a 64-bit kernel is then clearly wrong. > #endif /* _MIPS_SIM == _MIPS_SIM_NABI32 */ > > #define __NR_N32_Linux 6000 > -#define __NR_N32_Linux_syscalls332 > +#define __NR_N32_Linux_syscalls__NR_syscalls Same for this one. Arnd
Re: Official Linux system wrapper library?
On Wed, Nov 14, 2018 at 6:58 AM Carlos O'Donell wrote: > On 11/14/18 6:58 AM, Szabolcs Nagy wrote: > > an actual proposal in the thread that i think is > > worth considering is to make the linux syscall > > design process involve libc devs so the c api is > > designed together with the syscall abi. > > * Programmatic / Machine readable description of syscalls. > This way the kernel gives users the ability to autogenerate > all the wrappers *if they want to* in a consistent way that > matches this syscall description format. Firoz Khan is in the process of doing part of this, by changing the in-kernel per-architecture unistd.h and syscall.S files into a architecture independent machine-readable format that is used to generate the existing files. The format will be similar to what we have on arm/s390/x86 in the syscall.tbl files already. This is of course only part of the picture, it answers the question of which syscalls are implemented on an architecture, which number they have and (ideally) whether they use a standard implementation or a custom one, but it does not yet relate to the prototype. Once this work is merged, we can follow up by coming up with a way to add prototypes and enforcing that the user space wrapper uses the same argument types as the in-kernel entry point. Arnd
Re: [PATCH v2 2/9] arm64: defconfig: Drop ARM_BIG_LITTLE_CPUFREQ
On 11/10/18, Marc Gonzalez wrote:
> On 10/11/2018 09:57, Arnd Bergmann wrote:
>
>> On Thu, Nov 8, 2018 at 10:36 AM Sudeep Holla
>> wrote:
>>>
>>> On Wed, Nov 07, 2018 at 11:39:42PM +0100, Marc Gonzalez wrote:
>>>> Commit a7314405d83c ("drop ARM_BIG_LITTLE_CPUFREQ support for ARM64")
>>>> dropped ARM_BIG_LITTLE_CPUFREQ support for ARM64.
>>>>
>>>
>>> Looks good,
>>>
>>> Acked-by: Sudeep Holla
>>>
>>> But I left it intentionally to avoid churn assuming it will go away when
>>> ARM SoC team runs savedefconfig and sync the defconfig.
>>
>> I would never do that,
>
> Isn't that what commit c432c0880596 did? :-p
Olof did that one ;-)
Arnd
Re: [PATCH v2 2/9] arm64: defconfig: Drop ARM_BIG_LITTLE_CPUFREQ
On Thu, Nov 8, 2018 at 10:36 AM Sudeep Holla wrote:
>
> On Wed, Nov 07, 2018 at 11:39:42PM +0100, Marc Gonzalez wrote:
> > Commit a7314405d83c ("drop ARM_BIG_LITTLE_CPUFREQ support for ARM64")
> > dropped ARM_BIG_LITTLE_CPUFREQ support for ARM64.
> >
>
> Looks good,
>
> Acked-by: Sudeep Holla
>
> But I left it intentionally to avoid churn assuming it will go away when
> ARM SoC team runs savedefconfig and sync the defconfig.
I would never do that, instead I want patches like the one that Marc sent
in order to make sure we don't accidentally drop anything that is still
required, e.g. when an option got renamed or gained a dependency.
Arnd
Re: [PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus
On Fri, Nov 9, 2018 at 10:47 PM Sven Van Asbroeck wrote: > > On Fri, Nov 9, 2018 at 4:22 PM Arnd Bergmann wrote: > > > > > > As usual, it comes down to the user space interfaces I think. Designing > > a user interface is hard, most importantly because you cannot change it > > once anyone starts relying on it, as opposed to implementation details > > that you are free to change at any point. > > Thanks for the feedback ! > I will rework v4 so it uses a userspace interface shared with other > fieldbus drivers - uio. You mentioned this before, but I don't see how this would actually work. Isn't uio just a slim wrapper for allowing user space access to hardware registers while avoiding a kernel driver? Arnd
Re: [PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus
On Fri, Nov 9, 2018 at 5:02 PM Sven Van Asbroeck wrote: > > Arnd, Rob, Linus, > > Many thanks for your constructive feedback so far ! > > Is there anything in general about this set that would prevent it from being > mainlined? Perhaps I am trying to do too much at once, dropping a patchset > that is too complex to be properly reviewed? As usual, it comes down to the user space interfaces I think. Designing a user interface is hard, most importantly because you cannot change it once anyone starts relying on it, as opposed to implementation details that you are free to change at any point. It's also hard to find reviewers for it, the best you can hope for is that you find people that have both knowledge about the type of hardware you work with and Linux user interface design, and get them to review the code. > I've been thinking about reworking the host to its simplest, yet feature- > complete form. Just serialize all card accesses with a single lock. > Then the kernel thread, kqueue, kcache, kref etc would all disappear. > The price we pay is a reduction in performance/parallelism. > We could then increase parallelism at a later stage. > Would that be of any help? I don't think that's a major issue. If you are happy with the implementation there, and you use the interfaces correctly, that won't stop the driver from getting merged, even if you could rework it to use something else later. Arnd
Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus
On Fri, Nov 9, 2018 at 5:25 PM Sven Van Asbroeck wrote:
>
> On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann wrote:
> >
> > > +struct anybus_mbox_hdr {
> > > + u16 id;
> > > + u16 info;
> > > + u16 cmd_num;
> > > + u16 data_size;
> > > + u16 frame_count;
> > > + u16 frame_num;
> > > + u16 offset_high;
> > > + u16 offset_low;
> > > + u16 extended[8];
> > > +} __packed;
> >
> > I don't think you want this to be __packed, it won't change the layout
> > but instead force byte accesses on architectures that don't have
> > fast unaligned load/store instructions.
> >
> > Instead of the __packed, it's almost always better to ensure that
> > the structure itself is aligned to a 16-bit address.
> >
>
> A general question about __packed.
>
> My current understanding is this:
> (please tell me if it's incorrect or incomplete)
>
> + without __packed, the compiler is free to pad the struct in whatever
> way it feels is best.
> + with __packed, the fields have to be laid out EXACTLY as specified.
It's not up to the compiler but the ELF ABI. The rules are largely consisten
among the architectures we support, but there are a couple of notable
exceptions:
- ARM OABI requires 32-bit alignment for structures
- x86-32 aligns 64-bit members to 32-bit rather than 64-bit
- m68k has some oddities, I think they can pack certain
members (don't remember the details)
> Is it possible that someone will compile this on an architecture that 'likes'
> 16-bit ints to be 32-bit aligned? If such an architecture does not currently
> exist, could it appear in the future?
I'm fairly sure Linux would not be portable to an architecture like this
without a major development effort.
> If the compiler changes the padding in the struct, the driver will
> break, as the struct layout is tightly defined by the anybus spec.
>
> Would it be better to stay safe, and keep __packed in case of such
> tightly defined structures?
Generally I think it does more harm than good. If you require
__packed attributes for correct structure layout,
then only apply it to those members that are not naturally
aligned, and mark the structure itself as __aligned(n) with
the alignment you expect if that makes a difference.
Arnd
Re: [PATCH v6 5/9] dt-bindings: ARM: document marvell,ecc-enable binding
On Fri, Nov 9, 2018 at 12:48 PM Russell King - ARM Linux wrote: > > On Fri, Nov 09, 2018 at 12:40:06PM +0100, Arnd Bergmann wrote: > > On Fri, Nov 9, 2018 at 8:04 AM Chris Packham > > wrote: > > > > > > Add documentation for the marvell,ecc-enable and marvell,ecc-disable > > > properties which can be used to enable/disable ECC on the Marvell aurora > > > cache. > > > > > > Signed-off-by: Chris Packham > > > --- > > > > Why do you need both enable and disable? Wouldn't one of them be enough > > here? > > It isn't an "on when ecc-enable is present, off when not" because the > current behaviour is to preserve these bits in the control register. > > If we were to implement it as "if no ecc-enable property, turn off > ECC" then that would drastically change the behaviour - systems which > were configured for ECC suddenly lose ECC support. > > Since we don't know which have it and which don't, we can't implement > the option like that. What I meant was why we need support force-disabling it. I understand that we need to allow leaving it at the boot-time default as well as force-enabling it. Arnd
Re: [PATCH v6 5/9] dt-bindings: ARM: document marvell,ecc-enable binding
On Fri, Nov 9, 2018 at 8:04 AM Chris Packham wrote: > > Add documentation for the marvell,ecc-enable and marvell,ecc-disable > properties which can be used to enable/disable ECC on the Marvell aurora > cache. > > Signed-off-by: Chris Packham > --- Why do you need both enable and disable? Wouldn't one of them be enough here? Arnd
Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus
On Thu, Nov 8, 2018 at 4:47 PM Sven Van Asbroeck wrote: > On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann wrote: > > I see this is called from the interrupt handler at the moment, which > > means you cannot call sleeping functions, but it also means that > > the timeout may never happen because the timer tick IRQ cannot > > get through. That means you may have to change the irq handler > > logic, e.g. to try this a few times but then defer to a bottom half > > if it fails for a long time. > > Touche ! Yes, this is very likely a big problem. > > What if I converted the interrupt handler into a threaded interrupt handler? > That would allow the timer tick to get through, correct? Yes, with a threaded IRQ handler, the tick comes through, and you can use a sleeping function to back off between the retries. Arnd
Re: [PATCH anybus v3 6/6] misc: support HMS Profinet IRT industrial controller
On Thu, Nov 8, 2018 at 4:35 PM Sven Van Asbroeck wrote:
> On Thu, Nov 8, 2018 at 9:20 AM Arnd Bergmann wrote:
> > + struct {
> > > + /* addresses IN NETWORK ORDER! */
> > > + __u32 ip_addr;
> > > + __u32 subnet_msk;
> > > + __u32 gateway_addr;
> > > + __u8 is_valid:1;
> > > + } eth;
> >
> > Overall, this structure feels too complex for an ioctl interface,
> > with the nested structures. If we end up keeping that
> > ioctl, maybe at least make it a flat structure without padding,
> > or alternatively make it a set of separate ioctls.
> >
>
> I agree that it feels complex, but it's the best I could come up with.
> There are a few hidden constraints:
>
> 1. The profinet card configuration settings must be applied atomically.
> And they can only be applied right after device reset.
>
> So if we use smaller, separate ioctls, we will end up resetting the device
> each time we send an ioctl. And to assemble a real configuration, we need
> 5 or 6 ioctls, so the card gets reset 5 or 6 times, which takes ages.
> It cannot work this way.
>
> 2. Configuration settings are optional. That's why why each little struct
> has an is_valid member. If we use a flatter structure, we need a bool for
> every config setting in the struct, to indicate if it should be applied.
> Which feels clunky.
I think a more common way to do this would be to use a __u64
member containing a bitmask of which fields are valid.
> One way to overcome this is by letting the ioctls change data in the driver,
> but not on the card. Then a separate "apply" ioctl could apply the whole
> configuration atomically.
>
> Example:
> ioctl(clear all settings?);
> ioctl(set ip address);
> ioctl(set stop mode action);
> ioctl(enable internal webserver);
> ioctl(apply);
>
> But of course, what happens if two processes try to configure the
> driver at the same time?
> The ioctl calls would be interleaved, and the result would be very messy...
>
> There must be a better way?
In particular the bits about optional fields would fit much better
into netlink, which is a kind of TLV interface, and you can send
a number of configuration steps in one 'sendmsg' syscall,
but leave out the ones that you are not interested in.
This would also allow you to specify standard parameters that
could apply to multiple vendors or fieldbus protocols, and have
a common configuration tool for all of them.
Arnd
Re: [PATCH anybus v3 5/6] dt-bindings: anybuss-host: document devicetree binding
On Thu, Nov 8, 2018 at 3:21 PM Sven Van Asbroeck wrote: > > Hi Arnd, thank you for the review and the feedback ! > > > > > To allow describing connected devices, I think we need a #address-cells > > and #size-cells property here, with fixed values. > > I'm not sure I understand. Connected devices aren't described in the > devicetree. The anybus specification defines an id register, which is > then used to load the client driver automatically, in the manner of > pci/usb. > > In case I have misinterpreted your feedback, could you clarify a bit? This is related to my reply on another patch of this series. I think it's likely that you need to be able to describe anybus devices in DT for additional properties in some cases, even if the common case doesn't need it. We do the same thing on PCI and USB, where normally everything is probed through hardware access, but a device driver can look at dev->of_node to see if that contains any further properties. Arnd
Re: [PATCH anybus v3 6/6] misc: support HMS Profinet IRT industrial controller
On Sun, Nov 4, 2018 at 4:55 PM wrote:
> +
> +struct msgEthConfig {
> + u32 ip_addr, subnet_msk, gateway_addr;
> +} __packed;
The __packed attribute makes it slower to access but doesn't
change the layout, so better drop it.
> +struct msgMacAddr {
> + u8 addr[6];
> +} __packed;
> +
> +struct msgStr {
> + chars[128];
> +} __packed;
> +
> +struct msgShortStr {
> + chars[64];
> +} __packed;
> +
> +struct msgHicp {
> + charenable;
> +} __packed;
The __packed on these ones has no effect, and can just be dropped
for readability.
> +static ssize_t mac_addr_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct profi_priv *priv = dev_get_drvdata(dev);
> + struct msgMacAddr response;
> + int ret;
> +
> + ret = anybuss_recv_msg(priv->client, 0x0010, ,
> + sizeof(response));
> + if (ret)
> + return ret;
> + return snprintf(buf, PAGE_SIZE, "%02X:%02X:%02X:%02X:%02X:%02X\n",
> + response.addr[0], response.addr[1],
> + response.addr[2], response.addr[3],
> + response.addr[4], response.addr[5]);
> +}
> +
> +static DEVICE_ATTR_RO(mac_addr);
> +static ssize_t ip_addr_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
I don't understand much about field bus, but I have a general feeling
that if you configure a mac address and IP address, this should
be connect ed to the network layer in some form, possibly being
exposed as a network device and manipulated using netlink
instead of sysfs or ioctl.
Can you explain how this fits together and why you got the the
sysfs interface?
> +struct ProfinetConfig {
The CamelCase naming seems odd here.
> + struct {
> + /* addresses IN NETWORK ORDER! */
> + __u32 ip_addr;
> + __u32 subnet_msk;
> + __u32 gateway_addr;
> + __u8 is_valid:1;
> + } eth;
This is where packing might make sense, since you have
padding fields in a user space structure ;-) . It would be better
to just avoid the implicit padding though and name all fields.
Overall, this structure feels too complex for an ioctl interface,
with the nested structures. If we end up keeping that
ioctl, maybe at least make it a flat structure without padding,
or alternatively make it a set of separate ioctls.
Arnd
Re: [PATCH anybus v3 5/6] dt-bindings: anybuss-host: document devicetree binding
On Sun, Nov 4, 2018 at 4:55 PM wrote: > --- > .../bindings/bus/arcx,anybuss-host.txt| 36 +++ > 1 file changed, 36 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt > > diff --git a/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt > b/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt > new file mode 100644 > index ..5c28e4e09bb2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt > @@ -0,0 +1,36 @@ > +* Arcx Anybus-S host > + > +This host communicates with the SoC over a parallel bus. It is > +expected that its Device Tree node is specified as the child of a node > +corresponding to the parallel bus used for communication. > + > +Required properties: > + > + - compatible : The following string: > +"arcx,anybuss-host" > + > + - reg : bus memory area where the Anybus-S host dpram is located. > + > + - interrupts : interrupt connected to the Anybus-S host interrupt line. > + Generic interrupt client node bindings are described in > + interrupt-controller/interrupts.txt > + > + - resets : the reset line associated with the Anybus-S host. > + > +Example of usage: > + > +This example places the Anybus-S host on top of the i.MX WEIM parallel bus, > see: > +Documentation/devicetree/bindings/bus/imx-weim.txt To allow describing connected devices, I think we need a #address-cells and #size-cells property here, with fixed values. If a device is attached by a single 32-bit integer, then #address-cells = <1>; #size-cells = <0>; should be enough, otherwise you might need additional address cells. Actually listing the device(s) can be optional, but we do need the adress in order to let the bus code associate a device_node pointer with the child device structure to let a driver read the properties. Arnd
Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus
On Sun, Nov 4, 2018 at 4:55 PM wrote:
> From: Sven Van Asbroeck
> +struct anybus_mbox_hdr {
> + u16 id;
> + u16 info;
> + u16 cmd_num;
> + u16 data_size;
> + u16 frame_count;
> + u16 frame_num;
> + u16 offset_high;
> + u16 offset_low;
> + u16 extended[8];
> +} __packed;
I don't think you want this to be __packed, it won't change the layout
but instead force byte accesses on architectures that don't have
fast unaligned load/store instructions.
Instead of the __packed, it's almost always better to ensure that
the structure itself is aligned to a 16-bit address.
> +static struct ab_task *ab_task_create_get(struct kmem_cache *cache,
> + ab_task_fn_t task_fn)
> +{
> + struct ab_task *t;
> +
> + t = kmem_cache_alloc(cache, GFP_KERNEL);
> + if (!t)
> + return NULL;
> + t->cache = cache;
> + refcount_set(>refcnt, 1);
> + t->task_fn = task_fn;
> + t->done_fn = NULL;
> + t->result = 0;
> + init_completion(>done);
> + return t;
> +}
It looks like you build your own object management here. Maybe
use kobject, or at least kref instead of the refcount_t based
low-level implementation?
> +struct anybuss_host {
> + struct device *dev;
> + struct anybuss_client *client;
> + struct reset_control *reset;
> + struct regmap *regmap;
> + int irq;
> + struct task_struct *qthread;
> + wait_queue_head_t wq;
> + struct completion card_boot;
> + atomic_t ind_ab;
> + spinlock_t qlock;
> + struct kmem_cache *qcache;
> + struct kfifo qs[3];
> + struct kfifo *powerq;
> + struct kfifo *mboxq;
> + struct kfifo *areaq;
> + bool power_on;
> + bool softint_pending;
> + atomic_t dc_event;
> + wait_queue_head_t dc_wq;
> + atomic_t fieldbus_online;
> + struct kernfs_node *fieldbus_online_sd;
> +};
Similarly, should that anybuss_host sturcture maybe be based on
a 'struct device' itself? What is the hierarchy of devices in sysfs?
> +
> +static int read_ind_ab(struct regmap *regmap)
> +{
> + unsigned long timeout = jiffies + HZ/2;
> + unsigned int a, b;
> +
> + while (time_before_eq(jiffies, timeout)) {
> + regmap_read(regmap, REG_IND_AB, );
> + regmap_read(regmap, REG_IND_AB, );
> + if (likely(a == b))
> + return (int)a;
> + cpu_relax();
> + }
> + WARN(1, "IND_AB register not stable");
> + return -ETIMEDOUT;
> +}
500ms is an awfully long time for a busy loop. Can you change the
cpu_relax() into a msleep() or usleep_range() to let other processes
get some time?
I see this is called from the interrupt handler at the moment, which
means you cannot call sleeping functions, but it also means that
the timeout may never happen because the timer tick IRQ cannot
get through. That means you may have to change the irq handler
logic, e.g. to try this a few times but then defer to a bottom half
if it fails for a long time.
> +/* -- mailbox tasks --- */
> +
> +struct msgAnybusInit {
> + u16 input_io_len;
> + u16 input_dpram_len;
> + u16 input_total_len;
> + u16 output_io_len;
> + u16 output_dpram_len;
> + u16 output_total_len;
> + u16 op_mode;
> + u16 notif_config;
> + u16 wd_val;
> +} __packed;
Again, probably not __packed here.
> +static int task_fn_mbox_2(struct anybuss_host *cd, struct ab_task *t)
> +{
> + struct mbox_priv *pd = >mbox_pd;
> + unsigned int ind_ap;
> +
> + if (!cd->power_on)
> + return -EIO;
> + regmap_read(cd->regmap, REG_IND_AP, _ap);
> + if (((atomic_read(>ind_ab) ^ ind_ap) & IND_AX_MOUT) == 0) {
> + /* output message not here */
> + if (time_after(jiffies, t->start_jiffies + TIMEOUT))
> + return -ETIMEDOUT;
> + return -EINPROGRESS;
> + }
Again, avoid busy loops with long timeouts like this.
Arnd
Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge
On Tue, Nov 6, 2018 at 9:05 PM Sven Van Asbroeck wrote: > On Tue, Nov 6, 2018 at 1:31 PM Rob Herring wrote: > > > If the host is not a h/w component, but just a s/w protocol then it > > doesn't belong in DT. Perhaps it could be a library which the bridge > > driver can call into. > > Anybus cards have an id register, which identifies what they are, so > that the appropriate client driver may be instantiated. > In that sense anybus is very suited to the bus/client abstraction of > pci/usb/etc. > > > What are the resets connected to? The slots? Maybe you should model > > the slots in DT. > > > > Yes, the resets are ultimately connected to the slots. > I'm happy to model the slots in DT. It makes sense, they are physical, > hardware components. It may also be necessary to have a way to add more DT properties to a device in the slot that can not be probed by looking at the ID register. Ideally it should not be needed, but we have added this to most buses anyway: USB, PCI, MMC and others can all be probed automatically for many devices, but there are some devices that need a place where extra information can be stored, e.g. an ethernet MAC address (in the absence of a PROM), an external clock or reset line that is not part of the normal protocol, or a more specific identification when two devices accidentally have the same ID. Arnd
Re: [PATCH] riscv: add asm/unistd.h UAPI header
On Thu, Nov 8, 2018 at 3:10 AM Palmer Dabbelt wrote: > > On Wed, 07 Nov 2018 13:09:39 PST (-0800), Arnd Bergmann wrote: > > On Wed, Nov 7, 2018 at 7:30 PM David Abdurachmanov > > wrote: > >> On Wed, Nov 7, 2018 at 1:08 AM Palmer Dabbelt wrote: > >> > On Mon, 05 Nov 2018 12:56:15 PST (-0800), Arnd Bergmann wrote: > > > >> > The target is still the next glibc release (Feb 1st) for a stable RV32I > >> > ABI. > >> > That's progressing well, with one last blocking issue related to some of > >> > our > >> > floating-point emulation routines before we can submit the port. This > >> > should > >> > give us ample time to line up the ABIs correctly so everything works. > >> > > >> > So I think the correct answer here is to drop __ARCH_WANT_STAT64 from > >> > RISC-V. > >> > > >> > >> Then if you agree I could do and send v2: > >> > >> +#ifdef __LP64__ > >> +#define __ARCH_WANT_NEW_STAT > >> +#endif /* __LP64__ */ > > > > Looks good to me. > > This is a bit pedantic, but I'm not sure what the right answer is here: > "-march=rv64gc -mabi=ilp32d" will not define __LP64__, but will define > "__riscv_xlen == 64". I actually don't know enough about how an rv64gc/ilp32d > ABI would work to answer this: would we have "long long" all over our > syscalls? > > Probably not worth worrying about for now, as we'll have to go audit all of > these if we ever end up with an ilp32 ABI. So just go for it and we'll throw > this on the pile to deal with later :) Short answer: it doesn't matter because an ilp32d ABI would use neither newstat nor stat64, it would only need statx(). Long answer: We've gone through multiple iterations on the question. x86 uses long long in syscall interfaces and tries to reuse the native 64-bit syscalls as much as possible. This turned out to cause endless problems, so for the (never merged but still kept around as a patchset) arm64 ABI, we went the opposite way, and made the syscalls use the same ABI as the arm32 mode. >From the experience with both of the above, I'd say if you end up having to do it, use the same method as arm64, but try to resist doing it at all. Unlike arm64 and x86-64, there is no inherent benefit to using the 64-bit instruction set (doubled register number etc), so compared to the normal lp64 ABI you only gain a little dcache space for the smaller pointers at the cost of a smaller address space. For you as a maintainer however, the cost of supporting this mode is that you are stuck with three user space ABIs instead of just two (normal 32-bit and 64-bit). If anyone really wants to run 32-bit code, they need a CPU that allows switching modes. Arnd
Re: [PATCH 0/9] Regenerate arm64 defconfig for current kernel
On Wed, Nov 7, 2018 at 9:39 PM Marc Gonzalez wrote: > > Arnd, Olof, > > The following patch series was sent to a...@kernel.org > but the messages bounced because the SMTP server is > currently blacklisted. Can you get the patches through > the LAKML mailing list? Yes, that worked fine, and they ended up in the right folder for me because of the to:a...@kernel.org recipient. All patches look good to me, thanks a lot for doing this! One point about the changelogs: in some of them you just list the commit that changed things, but don't explain what the impact of that commit was (symbol dropped, always selected, etc). I always like to see changelog texts in full sentences, so if you can spend a few more minutes on this, would you send a v2 with that changed? Arnd
Re: [PATCH v2] ubsan: don't mark __ubsan_handle_builtin_unreachable as noreturn
On Wed, Nov 7, 2018 at 9:55 PM Andrew Morton wrote:
>
> On Wed, 7 Nov 2018 17:45:16 +0300 Andrey Ryabinin
> wrote:
>
> > From: Arnd Bergmann
> > --- a/lib/ubsan.c
> > +++ b/lib/ubsan.c
> > @@ -427,8 +427,7 @@ void __ubsan_handle_shift_out_of_bounds(struct
> > shift_out_of_bounds_data *data,
> > EXPORT_SYMBOL(__ubsan_handle_shift_out_of_bounds);
> >
> >
> > -void __noreturn
> > -__ubsan_handle_builtin_unreachable(struct unreachable_data *data)
> > +void __ubsan_handle_builtin_unreachable(struct unreachable_data *data)
> > {
> > unsigned long flags;
>
> This code has been here since 2016 and presumably people will want to
> build older kernels with newer gcc's. So I'll add cc:stable, OK?
>
Yes, sounds good. Thanks,
arnd
Re: [PATCH] riscv: add asm/unistd.h UAPI header
On Wed, Nov 7, 2018 at 7:30 PM David Abdurachmanov wrote: > On Wed, Nov 7, 2018 at 1:08 AM Palmer Dabbelt wrote: > > On Mon, 05 Nov 2018 12:56:15 PST (-0800), Arnd Bergmann wrote: > > The target is still the next glibc release (Feb 1st) for a stable RV32I ABI. > > That's progressing well, with one last blocking issue related to some of our > > floating-point emulation routines before we can submit the port. This > > should > > give us ample time to line up the ABIs correctly so everything works. > > > > So I think the correct answer here is to drop __ARCH_WANT_STAT64 from > > RISC-V. > > > > Then if you agree I could do and send v2: > > +#ifdef __LP64__ > +#define __ARCH_WANT_NEW_STAT > +#endif /* __LP64__ */ Looks good to me. > Cannot use CONFIG_64BIT as in user space nothing defines it. > Alternatively I could > check for __riscv_xlen == 64. > > I found _LP64 and __LP64__ being used in kernel, incl. > include/uapi/linux/rseq.h I think older gcc versions were less consistent about those macros, which is why I introduced __BITS_PER_LONG a long time ago. We now support gcc-4.6 as the minimum, so using __LP64__ is reliable, and on RISC-V we obviously have a much newer compiler anyway. Arnd
Re: [RFC 0/2] RISC-V: A proposal to add vendor-specific code
On 11/7/18, Palmer Dabbelt wrote: > On Mon, 05 Nov 2018 00:52:52 PST (-0800), Arnd Bergmann wrote: >> On 11/5/18, Christoph Hellwig wrote: >>> On Mon, Nov 05, 2018 at 02:58:07PM +0800, Vincent Chen wrote: >>>> Many thanks for kinds of comments. I quickly synthesize the comments >>>> and >>>> list them as below. >>>> 1. The kernel image shall include all vendor-specific code. >>> >>> I fundamentally disagree with this… and think it should be the contrary. >>> >>> 1. The kernel shall support no vendor specific instructions whatsoever, >>> period. >> >> I think what was meant above is >> >> 1. If a vendor extension requires kernel support, that support >> must be able to be built into a kernel image without breaking support >> for CPUs that do not have that extension, to allow building a single >> kernel image that works on all CPUs. > > Yes. I don't want anything that won't compile with upstream GCC, but I also > > don't want to have a Kconfig that says "make the kernel only work on > $VENDOR's implementation". I think this can be achieved, at least for the > cases I've seen so far. I think over time, the implementations will diverge. Ignoring the question of vendor extensions for the moment, you will definitely have to deal with combinations of (future) standard extensions. I can see two ways of doing that: Either each extension is a separate Kconfig option and you have to know which one to enable or disable for a particular target, or you list each platform separately with one Kconfig option, and have Kconfig/Kbuild work out which features to enable or disable based on that to get the fastest and most featureful kernel that still works on all enabled platforms. Arnd
Re: [PATCH] EDAC: skx_edac: add ACPI dependency
On Sat, Nov 3, 2018 at 12:42 AM Borislav Petkov wrote: > > On Fri, Nov 02, 2018 at 10:39:59AM -0700, Luck, Tony wrote: > > On Fri, Nov 02, 2018 at 05:10:13PM +0100, Borislav Petkov wrote: > > > On Fri, Nov 02, 2018 at 04:32:06PM +0100, Arnd Bergmann wrote: > > > > We cannot currently select ACPI_ADXL without also enabling the top-level > > I have to say that Arnd's fix is prettier. With it, we can go back > > to > > > > select ACPI_ADXL > > > > instead of > > > > select ACPI_ADXL if ACPI > > Arnd's fix already has that last line in there: > > config EDAC_SKX > tristate "Intel Skylake server Integrated MC" > - depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG > + depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG && ACPI > depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_SKX can't > be y > select DMI > select ACPI_ADXL if ACPI > > > so I'm reading this as *additionally* needed, ontop of the ugly fix. > > But let's wait until he clarifies first. No, it was unintentional, the 'if ACPI' can be dropped when we add 'depends on ACPI'. Arnd
Re: [PATCH v1 4/4] xtensa: generate uapi header and syscall table header files
On Mon, Nov 5, 2018 at 10:44 PM Max Filippov wrote: > On Thu, Nov 1, 2018 at 6:57 AM Firoz Khan wrote: > > index 82c75643..b35d2e6 100644 > > --- a/arch/xtensa/include/asm/Kbuild > > +++ b/arch/xtensa/include/asm/Kbuild > > @@ -28,3 +28,4 @@ generic-y += topology.h > > generic-y += trace_clock.h > > generic-y += word-at-a-time.h > > generic-y += xor.h > > +generic-y += syscall_table.h > > This doesn't look right: syscall_table.h is not asm-generic header. > > > diff --git a/arch/xtensa/include/uapi/asm/Kbuild > > b/arch/xtensa/include/uapi/asm/Kbuild > > index 837d4dd..f826d52 100644 > > --- a/arch/xtensa/include/uapi/asm/Kbuild > > +++ b/arch/xtensa/include/uapi/asm/Kbuild > > @@ -11,3 +11,4 @@ generic-y += resource.h > > generic-y += siginfo.h > > generic-y += statfs.h > > generic-y += termios.h > > +generic-y += unistd_32.h > > Ditto: unistd32_.h is not asm-generic header. I think the correct syntax here would be generated-y += unistd_32.h Firoz, if I remember correctly, this came up before, maybe you just did not update all architectures together? Arnd
Re: [PATCH RFC 2/3] mmc: sdhci-omap: Add using external dma
On 11/5/18, Kishon Vijay Abraham I wrote: > On 05/11/18 8:46 AM, Chunyan Zhang wrote: >> >> +sdhci_switch_extdma(host, true); > > A number of devices using sdhci-omap supports ADMA. So switching to > external > DMA shouldn't be unconditional. > > IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it > should > try switching to external DMA and if external DMA too is not supported, it > should use PIO. What's the reasoning for preferring ADMA/SDMA over external DMA if both are supported? I'd expect that if the external DMA for some reason is worse than ADMA, we just wouldn't list it in the DT at all, but if it's listed and ADMA also works, then the driver should try to use it before falling back to ADMA. Arnd
Re: [PATCH RFC 1/3] mmc: sdhci: add support for using external DMA devices
On 11/5/18, Chunyan Zhang wrote:
> Some standard SD host controller can support both external dma
> controllers as well as ADMA in which the controller acts as
> DMA master.
>
> Currently the generic SDHCI code supports ADMA/SDMA integrated into
> the host controller but does not have any support for external DMA
> controllers implemented using dmaengine meaning that custom code is
> needed for any systems that use a generic DMA controller with SDHCI.
>
> Signed-off-by: Chunyan Zhang
Looks good to me overall, but I think I found one small bug:
> + dma->rx_chan = dma_request_chan(mmc->parent, "rx");
> + if (IS_ERR(dma->rx_chan)) {
> + ret = PTR_ERR(dma->rx_chan);
> + if (ret == -EPROBE_DEFER && dma->tx_chan)
> + dma_release_channel(dma->tx_chan);
> +
> + dma->rx_chan = NULL;
> + pr_warn("Failed to request RX DMA channel.\n");
> + }
The error handling looks wrong here: if you get EPROBE_DEFER,
you want to skip the warning message. If you get any other error code,
you want the warning message and also the dma_release_channel()
which should be unconditional here.
Arnd
Re: [alsa-devel] [PATCH] ASoC: skl: always select SND_SOC_HDAC_HDA
On 11/5/18, Andy Shevchenko wrote:
> On Mon, Nov 5, 2018 at 7:19 PM Pierre-Louis Bossart
> wrote:
>>
>>
>> >>> We have this ("strange") lines over the drivers:
>> >>>
>> >>> config BAR
>> >>> depends on FOO || FOO=n
>> >>>
>> >>> which guarantees that FOO will be not module when BAR is built-in.
>> >> That's what I normally use, but I could not figure this one out.
>> >> One problem is that SND_SOC_ALL_CODECS selects
>> >> SND_SOC_HDAC_HDA, and SND_SOC_ALL_CODECS itself
>> >> may be =m, causing the failure for
>> >> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=y.
>> >>
>> >> It might work with a separate dummy symbol:
>> >>
>> >> config SND_SOC_HDAC_HDA_FORCE
>> >> tristate
>
>> >> depends on SND_SOC_ALL_CODECS != n
>
> Forgot to mention that AFAIU above line is no-op in symbols which are
> hidden.
>
It's not. In this case, it will prevent SND_SOC_HDAC_HDA_FORCE
from being turned on by the 'default ...' line. In other cases, it can
generate a warning if you have a hidden symbol that is selected
by some other symbol while the dependency is missing.
Arnd
Re: [PATCH] riscv: add asm/unistd.h UAPI header
On 11/5/18, David Abdurachmanov wrote:
> Marcin Juszkiewicz reported issues while generating syscall table for riscv
> using 4.20-rc1. The patch refactors our unistd.h files to match some other
> architectures.
>
> - Add asm/unistd.h UAPI header, which has __ARCH_WANT_NEW_STAT
> - Remove asm/syscalls.h UAPI header and merge to asm/unistd.h
> - Adjust kernel asm/unistd.h
>
> So now asm/unistd.h UAPI header should show all syscalls for riscv.
>
> Before this, Makefile simply put `#include ` into
> generated asm/unistd.h UAPI header thus user didn't see:
>
> - __NR_riscv_flush_icache
> - __NR_newfstatat
> - __NR_fstat
>
> which are supported by riscv kernel.
>
> Signed-off-by: David Abdurachmanov
> Cc: Arnd Bergmann
> Cc: Marcin Juszkiewicz
> Cc: Guenter Roeck
Thanks for addressing this, your patch correctly fixes riscv64, and
I should have noticed the mistake when I originally merged the
broken patch.
However, looking closer I found another problem with the original
patch that your fix does not address:
__ARCH_WANT_NEW_STAT should only be set on 64-bit
architectures.
For a 32-bit architecture, we only want __ARCH_WANT_STAT64 if
any. For 64-bit architectures with compat mode, we still need to
set __ARCH_WANT_STAT64 from the non-uapi file so we get
the syscall implementation.
If we don't care about the riscv32 ABI changing yet, we can
decide to leave out __ARCH_WANT_STAT64 here, and require
glibc to implement it using statx() like any new architecture.
stat64 is not y2038 safe, and statx replaces it because of that.
> Fixes: 67314ec7b025
That line should be formatted as
Fixes: 67314ec7b025 ("RISC-V: Request newstat syscalls")
Re: [PATCH] mm: fix uninitialized variable warnings
On 11/5/18, Jan Kara wrote: > On Fri 02-11-18 16:31:06, Arnd Bergmann wrote: >> In a rare randconfig build, I got a warning about possibly uninitialized >> variables: >> >> mm/page-writeback.c: In function 'balance_dirty_pages': >> mm/page-writeback.c:1623:16: error: 'writeback' may be used uninitialized >> in this function [-Werror=maybe-uninitialized] >> mdtc->dirty += writeback; >> ^~ >> mm/page-writeback.c:1624:4: error: 'filepages' may be used uninitialized >> in this function [-Werror=maybe-uninitialized] >> mdtc_calc_avail(mdtc, filepages, headroom); >> ^~ >> mm/page-writeback.c:1624:4: error: 'headroom' may be used uninitialized in >> this function [-Werror=maybe-uninitialized] >> >> The compiler evidently fails to notice that the usage is in dead code >> after 'mdtc' is set to NULL when CONFIG_CGROUP_WRITEBACK is disabled. >> Adding an IS_ENABLED() check makes this clear to the compiler. >> >> Signed-off-by: Arnd Bergmann > > I'm surprised the compiler was not able to infer this since: > > struct dirty_throttle_control * const mdtc = mdtc_valid(_stor) ? > _stor : NULL; > > and if CONFIG_CGROUP_WRITEBACK is disabled, mdtc_valid() is defined to > 'false'. But possibly the function is just too big and the problematic > condition is in the loop so maybe it all confuses the compiler too much. On second thought, I suspect this started with the introduction of CONFIG_NO_AUTO_INLINE in linux-next. That also caused a similar issue in 28 other files that I patched later. I wrote this patch before I saw the others, and then didn't make the connection. Let's drop the patch for now, and decide what we want to do for the others. I fixed those by adding 'inline' markers for whatever function needed it. Arnd
Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency
On 11/5/18, Andy Shevchenko wrote: > On Fri, Nov 02, 2018 at 11:07:34PM +0100, Arnd Bergmann wrote: >> On 11/2/18, Andy Shevchenko wrote: >> > On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote: > >> >> - depends on MFD_AXP20X_I2C && IOSF_MBI >> >> + depends on MFD_AXP20X_I2C && IOSF_MBI=y >> > >> > To me sounds like >> > >> > select IOSF_MBI would be more appropriate here. >> >> It looks like we have a mix of the two two, with most drivers >> using 'select' and only a few ones using 'depends on'. Mixing >> the two often leads to trouble, especially for user-visible >> symbols. >> >> Making it a hidden symbol that is always selected is probably >> fine, but then every driver selecting it must also use 'depends >> on X86 && PCI'. > > I doubt every is a correct word here. Whenever driver uses IOSF_MBI it > implies X86 and PCI (or should have those dependencies in mind already). I mean it must depend on those two in some form. If a driver uses 'depends on IOSF_MBI' today, that is implied through that dependency. Changing it to 'select' means we have to add that dependency, like diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 56ccb1ea7da5..fb750a8a9b77 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -489,6 +489,7 @@ config I2C_DESIGNWARE_PLATFORM tristate "Synopsys DesignWare Platform" select I2C_DESIGNWARE_CORE depends on (ACPI && COMMON_CLK) || !ACPI + select IOSF_MBI if I2C_DESIGNWARE_BAYTRAIL help If you say yes to this option, support will be included for the Synopsys DesignWare I2C adapter. @@ -520,9 +521,8 @@ config I2C_DESIGNWARE_PCI config I2C_DESIGNWARE_BAYTRAIL bool "Intel Baytrail I2C semaphore support" - depends on ACPI - depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \ - (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y) + depends on ACPI && X86 && PCI + depends on I2C_DESIGNWARE_PLATFORM help This driver enables managed host access to the PMIC I2C bus on select Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows For anything that already has the dependency, nothing changes. Arnd
Re: [alsa-devel] [PATCH] ASoC: skl: always select SND_SOC_HDAC_HDA
On 11/5/18, Andy Shevchenko wrote:
> On Sun, Nov 04, 2018 at 10:45:17AM -0600, Pierre-Louis Bossart wrote:
>
>> So yes indeed we have to add a select HDAC_HDA statement under the
>> SKYLAKE
>> config - i just don't know of any other means to say "don't build
>> HDAC_HDA
>> as a module when SKYLAKE is buit-in"
>
> We have this ("strange") lines over the drivers:
>
> config BAR
> depends on FOO || FOO=n
>
> which guarantees that FOO will be not module when BAR is built-in.
That's what I normally use, but I could not figure this one out.
One problem is that SND_SOC_ALL_CODECS selects
SND_SOC_HDAC_HDA, and SND_SOC_ALL_CODECS itself
may be =m, causing the failure for
SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=y.
It might work with a separate dummy symbol:
config SND_SOC_HDAC_HDA_FORCE
tristate
depends on SND_SOC_ALL_CODECS != n
default SND_SOC_INTEL_SKYLAKE
select SND_SOC_HDAC_HDA
This would make SND_SOC_HDAC_HDA built-in even
with SND_SOC_ALL_CODECS=m and
SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=n
It seems a bit ugly, and would need some testing.
Arnd
Re: [PATCH] x86: vsmp: avoid link error with gcc -Og
On 11/5/18, Thomas Gleixner wrote: > Arnd, > > On Fri, 2 Nov 2018, Arnd Bergmann wrote: > >> When the compiler skips optimizations for ease of debugging, >> teh vsmp_cap_cpus() function is not eliminated from the build >> with CONFIG_PCI=n: > > thanks for that, but the code has other issues, which are addressed with > this: > > > https://lkml.kernel.org/r/1541404864-31603-1-git-send-email-e...@scalemp.com > > That should also solve the problem you've observed. Ok, I've dropped my patch from my test tree, will let you know if I see any other issues with the new patch applied. Arnd
Re: [RFC 0/2] RISC-V: A proposal to add vendor-specific code
On 11/5/18, Christoph Hellwig wrote: > On Mon, Nov 05, 2018 at 09:52:52AM +0100, Arnd Bergmann wrote: >> > I fundamentally disagree with this… and think it should be the >> > contrary. >> > >> > 1. The kernel shall support no vendor specific instructions whatsoever, >> > period. >> >> I think what was meant above is >> >> 1. If a vendor extension requires kernel support, that support >> must be able to be built into a kernel image without breaking support >> for CPUs that do not have that extension, to allow building a single >> kernel image that works on all CPUs. > > No. This literally means no vendor extensions involving instructions > or CSRs in the kernel. They are fine for userspace, or for the M-mode > code including impementation of the SBI, but not for the kernel. I was trying to interpret what Vincent wrote, not what you wrote, you were pretty clear already ;-) With the stricter policy you suggest, we'd loose the ability to support some extensions that might be common: - an extension for user space that adds new registers that must be saved and restored on a task switch, e.g. FPU, DSP or NPU instructions. ARM supports several incompatible extensions like that in one kernel, and this is really ugly, but I suspect RISC-V will already need the same thing to support all combinations of standard extensions, so from a practical perspective it's not much different for custom extension, aside from the question how far you want to go to discourage custom extensions by requiring users to patch their kernels. - A crypto instruction for a cipher that is used in the kernel for speeding up network or block data encryption. This would typically be a standalone loadable module, so the impact of allowing custom extensions in addition to standard ones is minimal. Arnd
Re: [RFC 0/2] RISC-V: A proposal to add vendor-specific code
On 11/5/18, Christoph Hellwig wrote: > On Mon, Nov 05, 2018 at 02:58:07PM +0800, Vincent Chen wrote: >> Many thanks for kinds of comments. I quickly synthesize the comments and >> list them as below. >> 1. The kernel image shall include all vendor-specific code. > > I fundamentally disagree with this… and think it should be the contrary. > > 1. The kernel shall support no vendor specific instructions whatsoever, > period. I think what was meant above is 1. If a vendor extension requires kernel support, that support must be able to be built into a kernel image without breaking support for CPUs that do not have that extension, to allow building a single kernel image that works on all CPUs. Arnd
[PATCH] ASoC: Intel: mrfld: fix uninitialized variable access
Randconfig testing revealed a very old bug, with gcc-8:
sound/soc/intel/atom/sst/sst_loader.c: In function 'sst_load_fw':
sound/soc/intel/atom/sst/sst_loader.c:357:5: error: 'fw' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
if (fw == NULL) {
^
sound/soc/intel/atom/sst/sst_loader.c:354:25: note: 'fw' was declared here
const struct firmware *fw;
We must check the return code of request_firmware() before we look at the
pointer result that may be uninitialized when the function fails.
Fixes: 9012c9544eea ("ASoC: Intel: mrfld - Add DSP load and management")
Signed-off-by: Arnd Bergmann
---
sound/soc/intel/atom/sst/sst_loader.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/atom/sst/sst_loader.c
b/sound/soc/intel/atom/sst/sst_loader.c
index 27413ebae956..b8c456753f01 100644
--- a/sound/soc/intel/atom/sst/sst_loader.c
+++ b/sound/soc/intel/atom/sst/sst_loader.c
@@ -354,14 +354,14 @@ static int sst_request_fw(struct intel_sst_drv *sst)
const struct firmware *fw;
retval = request_firmware(, sst->firmware_name, sst->dev);
- if (fw == NULL) {
- dev_err(sst->dev, "fw is returning as null\n");
- return -EINVAL;
- }
if (retval) {
dev_err(sst->dev, "request fw failed %d\n", retval);
return retval;
}
+ if (fw == NULL) {
+ dev_err(sst->dev, "fw is returning as null\n");
+ return -EINVAL;
+ }
mutex_lock(>sst_lock);
retval = sst_cache_and_parse_fw(sst, fw);
mutex_unlock(>sst_lock);
--
2.18.0
Re: [PATCH] copy_{to,from}_user(): fix compile-time sanity checks with gcc -Og
On 11/3/18, Changbin Du wrote: > On Fri, Nov 02, 2018 at 04:56:58PM +0100, Arnd Bergmann wrote: >> When CONFIG_CC_OPTIMIZE_FOR_DEBUGGING is set, we get countless warnings >> like >> >> In function 'check_copy_size', >> inlined from 'copy_from_user' at include/linux/uaccess.h:146:6, >> inlined from '__copy_siginfo_from_user' at kernel/signal.c:3032:6: >> include/linux/thread_info.h:147:4: error: call to '__bad_copy_to' declared >> with attribute error: copy destination size is too small >> >> It seems that constant propagation doesn't work well enough to make >> this code reliable, so turn it off for that configuration. >> > This is caused by __compiletime_warning() and fixed by below change > already. Could you try the latest kbuild tree? > > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -86,8 +86,10 @@ > #define __compiletime_object_size(obj) __builtin_object_size(obj, 0) > > #ifndef __CHECKER__ > +#ifndef CONFIG_CC_OPTIMIZE_FOR_DEBUGGING > #define __compiletime_warning(message) __attribute__((warning(message))) > #define __compiletime_error(message) __attribute__((error(message))) > +#endif Right, that works as well, but my version is a bit less invasive as it only disables the __compiletime_error in the one file that introduced the regression, and not all of them. I have built hundreds of randconfig builds and found no other problem with __compiletime_error(), though I did find a bunch of (correct) section mismatch warnings and false-positive -Wformat-overflow= warnings with CONFIG_CC_OPTIMIZE_FOR_DEBUGGING (patches pending). I also got some link errors for which I already posted patches. Arnd
Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency
On 11/2/18, Arnd Bergmann wrote:
> On 11/2/18, Andy Shevchenko wrote:
>> On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote:
>>> We still get a link failure with IOSF_MBI=m when the xpower driver
>>> is built-in:
>>>
>>> drivers/acpi/pmic/intel_pmic_xpower.o: In function
>>> `intel_xpower_pmic_update_power':
>>> intel_pmic_xpower.c:(.text+0x4f2): undefined reference to
>>> `iosf_mbi_block_punit_i2c_access'
>>> intel_pmic_xpower.c:(.text+0x5e2): undefined reference to
>>> `iosf_mbi_unblock_punit_i2c_access'
>>>
>>> This makes the dependency stronger, so we can only build when IOSF_MBI
>>> is built-in.
>>>
>>> Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to
>>> Kconfig entry")
>>> Signed-off-by: Arnd Bergmann
>>> ---
>>> drivers/acpi/Kconfig | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>> index 18851e7eedd5..31a3c4a03f61 100644
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION
>>>
>>> config XPOWER_PMIC_OPREGION
>>> bool "ACPI operation region support for XPower AXP288 PMIC"
>>> - depends on MFD_AXP20X_I2C && IOSF_MBI
>>> + depends on MFD_AXP20X_I2C && IOSF_MBI=y
>>
>> To me sounds like
>>
>> select IOSF_MBI would be more appropriate here.
>
> It looks like we have a mix of the two two, with most drivers
> using 'select' and only a few ones using 'depends on'. Mixing
> the two often leads to trouble, especially for user-visible
> symbols.
>
> Making it a hidden symbol that is always selected is probably
> fine, but then every driver selecting it must also use 'depends
> on X86 && PCI'.
Oh, and that also requires removing the #else section in
arch/x86/include/asm/iosf_mbi.h, otherwise you might have
a driver that can build with or without CONFIG_IOSF_MBI,
but fails to be built-in when some other tristate symbol
uses 'select IOSF_MBI'. Making it a 'bool' seems easier
there.
Arnd
Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency
On 11/2/18, Andy Shevchenko wrote:
> On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote:
>> We still get a link failure with IOSF_MBI=m when the xpower driver
>> is built-in:
>>
>> drivers/acpi/pmic/intel_pmic_xpower.o: In function
>> `intel_xpower_pmic_update_power':
>> intel_pmic_xpower.c:(.text+0x4f2): undefined reference to
>> `iosf_mbi_block_punit_i2c_access'
>> intel_pmic_xpower.c:(.text+0x5e2): undefined reference to
>> `iosf_mbi_unblock_punit_i2c_access'
>>
>> This makes the dependency stronger, so we can only build when IOSF_MBI
>> is built-in.
>>
>> Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to
>> Kconfig entry")
>> Signed-off-by: Arnd Bergmann
>> ---
>> drivers/acpi/Kconfig | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 18851e7eedd5..31a3c4a03f61 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION
>>
>> config XPOWER_PMIC_OPREGION
>> bool "ACPI operation region support for XPower AXP288 PMIC"
>> -depends on MFD_AXP20X_I2C && IOSF_MBI
>> +depends on MFD_AXP20X_I2C && IOSF_MBI=y
>
> To me sounds like
>
> select IOSF_MBI would be more appropriate here.
It looks like we have a mix of the two two, with most drivers
using 'select' and only a few ones using 'depends on'. Mixing
the two often leads to trouble, especially for user-visible
symbols.
Making it a hidden symbol that is always selected is probably
fine, but then every driver selecting it must also use 'depends
on X86 && PCI'.
Arnd
Re: [PATCH] ASoC: sunxi: rename SND_SUN8I_ADDA_PR_REGMAP
On 11/2/18, Vasily Khoruzhick wrote: > On Friday, November 2, 2018 8:33:40 AM PDT Arnd Bergmann wrote: >> The newly added SND_SUN50I_CODEC_ANALOG driver selects a nonexisting >> symbol SND_SUNXI_ADDA_PR_REGMAP, which was evidently intended to replace >> SND_SUN8I_ADDA_PR_REGMAP, but since they are now mismatched, we can run >> into link errors for some configurations: >> >> sound/soc/sunxi/sun50i-codec-analog.o: In function >> `sun50i_codec_analog_probe': sun50i-codec-analog.c:(.text+0x62): >> undefined >> reference to `sun8i_adda_pr_regmap_init' >> >> The new name appears to be more sensible, and as the symbol is hidden, >> there are no downsides in the rename, so use that consistently now. > > Maxime asked it to be SND_SUN8I_ADDA_PR_REGMAP, and also it would be easier > to > fix it my renaming last remaining SND_SUNXI_ADDA_PR_REGMAP to > SND_SUN8I_ADDA_PR_REGMAP. Sure, I don't care either way. Please send a patch to do that then. Arnd
Re: [PATCH] ASoC: skl: always select SND_SOC_HDAC_HDA
On 11/2/18, Pierre-Louis Bossart wrote: > > On 11/2/18 6:24 AM, Arnd Bergmann wrote: >> The skylake sound support is written to work both with or without >> CONFIG_SND_SOC_HDAC_HDA, and uses an #ifdef to decide whether it should >> link against that. However, this fails with SND_SOC_ALL_CODECS=m or >> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=m when the Skylake support itself >> is built-in, with this link error: >> >> sound/soc/intel/skylake/skl.o: In function `skl_probe': >> skl.c:(.text+0x56c): undefined reference to `snd_soc_hdac_hda_get_ops' >> >> Using an explicit 'select' here simplifies the logic and prevents >> it from happening, at the cost of always including the compile >> time dependency. > > Thanks Arnd for the report. I don't quite agree with the proposal, this > should be similar to HDAC_HDMI which is not selected by default, and > there's no reason to force the support for HDAudio when the vast > majority of Skylake+ devices which enable this driver don't have any > HDaudio codec. Sure, feel free to treat this patch as a bug report and come up with a better fix. > Also the code is skl.c is already conditional, so this function should > not be required. > > Do you have a config that fails, I'd like to look at this further. https://pastebin.com/4iYPGxkU is what first triggered this. Arnd
[PATCH] copy_{to,from}_user(): fix compile-time sanity checks with gcc -Og
When CONFIG_CC_OPTIMIZE_FOR_DEBUGGING is set, we get countless warnings
like
In function 'check_copy_size',
inlined from 'copy_from_user' at include/linux/uaccess.h:146:6,
inlined from '__copy_siginfo_from_user' at kernel/signal.c:3032:6:
include/linux/thread_info.h:147:4: error: call to '__bad_copy_to' declared with
attribute error: copy destination size is too small
It seems that constant propagation doesn't work well enough to make
this code reliable, so turn it off for that configuration.
Cc: Changbin Du
Cc: Al Viro
Signed-off-by: Arnd Bergmann
---
include/linux/thread_info.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 8d8821b3689a..762f379bdf5d 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -138,6 +138,11 @@ static __always_inline bool
check_copy_size(const void *addr, size_t bytes, bool is_source)
{
int sz = __compiletime_object_size(addr);
+
+ /* constant propagation doesn't work well with -Og */
+ if (IS_ENABLED(CONFIG_CC_OPTIMIZE_FOR_DEBUGGING))
+ return true;
+
if (unlikely(sz >= 0 && sz < bytes)) {
if (!__builtin_constant_p(bytes))
copy_overflow(sz, bytes);
--
2.18.0
Re: linux-next: Tree for Oct 31 (vboxguest)
On 11/2/18, Masahiro Yamada wrote: > On Thu, Nov 1, 2018 at 11:32 PM Changbin Du wrote: >> On Thu, Nov 01, 2018 at 12:32:48PM +0900, Masahiro Yamada wrote: > > How about clang? > > For clang, -Og might be equivalent to -O1 at this moment, but I am not > sure. > > In my understanding, Clang does not inline functions marked with 'static > inline' > for -Og (or -O1) optimization level. > > Theoretically, 'inline' keyword is a just hint for the compiler, after all. I think this means that we cannot build the kernel in that configuration, at least with CONFIG_OPTIMIZE_INLINING=y. Without that option, every 'inline' becomes 'always_inline'. Arnd
[PATCH] x86: vsmp: avoid link error with gcc -Og
When the compiler skips optimizations for ease of debugging,
teh vsmp_cap_cpus() function is not eliminated from the build
with CONFIG_PCI=n:
arch/x86/kernel/vsmp_64.o: In function `vsmp_cap_cpus':
vsmp_64.c:(.init.text+0x23): undefined reference to `read_pci_config'
Change the caller in a way that will always optimize this
as required.
Signed-off-by: Arnd Bergmann
---
arch/x86/kernel/vsmp_64.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/vsmp_64.c b/arch/x86/kernel/vsmp_64.c
index 1eae5af491c2..6a2ae6964ce5 100644
--- a/arch/x86/kernel/vsmp_64.c
+++ b/arch/x86/kernel/vsmp_64.c
@@ -146,8 +146,10 @@ static void __init detect_vsmp_box(void)
is_vsmp = 1;
}
-static int is_vsmp_box(void)
+static int __init is_vsmp_box(void)
{
+ detect_vsmp_box();
+
if (is_vsmp != -1)
return is_vsmp;
else {
@@ -157,10 +159,7 @@ static int is_vsmp_box(void)
}
#else
-static void __init detect_vsmp_box(void)
-{
-}
-static int is_vsmp_box(void)
+static __always_inline int is_vsmp_box(void)
{
return 0;
}
@@ -213,7 +212,6 @@ static void vsmp_apic_post_init(void)
void __init vsmp_init(void)
{
- detect_vsmp_box();
if (!is_vsmp_box())
return;
--
2.18.0
[PATCH] ubifs: replay: fix high stack usage
Having two shash descriptors on the stack cause a very significant kernel
stack usage that can cross the warning threshold:
fs/ubifs/replay.c: In function 'authenticate_sleb':
fs/ubifs/replay.c:633:1: error: the frame size of 1144 bytes is larger than
1024 bytes [-Werror=frame-larger-than=]
Normally, gcc optimizes the out, but with CONFIG_CC_OPTIMIZE_FOR_DEBUGGING,
it does not. Splitting the two stack allocations into separate functions
means that they will use the same memory again. In normal configurations
(optimizing for size or performance), those should get inlined and we get
the same behavior as before.
Signed-off-by: Arnd Bergmann
---
fs/ubifs/replay.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c
index 75f961c4c044..a08c5b7030ea 100644
--- a/fs/ubifs/replay.c
+++ b/fs/ubifs/replay.c
@@ -533,6 +533,28 @@ static int is_last_bud(struct ubifs_info *c, struct
ubifs_bud *bud)
return data == 0x;
}
+/* authenticate_sleb_hash and authenticate_sleb_hmac are split out for stack
usage */
+static int authenticate_sleb_hash(struct ubifs_info *c, struct shash_desc
*log_hash, u8 *hash)
+{
+ SHASH_DESC_ON_STACK(hash_desc, c->hash_tfm);
+
+ hash_desc->tfm = c->hash_tfm;
+ hash_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+
+ ubifs_shash_copy_state(c, log_hash, hash_desc);
+ return crypto_shash_final(hash_desc, hash);
+}
+
+static int authenticate_sleb_hmac(struct ubifs_info *c, u8 *hash, u8 *hmac)
+{
+ SHASH_DESC_ON_STACK(hmac_desc, c->hmac_tfm);
+
+ hmac_desc->tfm = c->hmac_tfm;
+ hmac_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+
+ return crypto_shash_digest(hmac_desc, hash, c->hash_len, hmac);
+}
+
/**
* authenticate_sleb - authenticate one scan LEB
* @c: UBIFS file-system description object
@@ -574,21 +596,12 @@ static int authenticate_sleb(struct ubifs_info *c, struct
ubifs_scan_leb *sleb,
if (snod->type == UBIFS_AUTH_NODE) {
struct ubifs_auth_node *auth = snod->node;
- SHASH_DESC_ON_STACK(hash_desc, c->hash_tfm);
- SHASH_DESC_ON_STACK(hmac_desc, c->hmac_tfm);
-
- hash_desc->tfm = c->hash_tfm;
- hash_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
- ubifs_shash_copy_state(c, log_hash, hash_desc);
- err = crypto_shash_final(hash_desc, hash);
+ err = authenticate_sleb_hash(c, log_hash, hash);
if (err)
goto out;
- hmac_desc->tfm = c->hmac_tfm;
- hmac_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
- err = crypto_shash_digest(hmac_desc, hash, c->hash_len,
- hmac);
+ err = authenticate_sleb_hmac(c, hash, hmac);
if (err)
goto out;
--
2.18.0
[PATCH] vbox: fix link error with 'gcc -Og'
With the new CONFIG_CC_OPTIMIZE_FOR_DEBUGGING option, we get a link error in the vboxguest driver, when that fails to optimize out the call to the compat handler: drivers/virt/vboxguest/vboxguest_core.o: In function `vbg_ioctl_hgcm_call': vboxguest_core.c:(.text+0x1f6e): undefined reference to `vbg_hgcm_call32' Another compile-time check documents better what we want and avoids the error. Acked-by: Randy Dunlap Tested-by: Randy Dunlap Signed-off-by: Arnd Bergmann --- drivers/virt/vboxguest/vboxguest_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virt/vboxguest/vboxguest_core.c b/drivers/virt/vboxguest/vboxguest_core.c index 3093655c7b92..1475ed5ffcde 100644 --- a/drivers/virt/vboxguest/vboxguest_core.c +++ b/drivers/virt/vboxguest/vboxguest_core.c @@ -1312,7 +1312,7 @@ static int vbg_ioctl_hgcm_call(struct vbg_dev *gdev, return -EINVAL; } - if (f32bit) + if (IS_ENABLED(CONFIG_COMPAT) && f32bit) ret = vbg_hgcm_call32(gdev, client_id, call->function, call->timeout_ms, VBG_IOCTL_HGCM_CALL_PARMS32(call), -- 2.18.0
[PATCH] ASoC: sunxi: rename SND_SUN8I_ADDA_PR_REGMAP
The newly added SND_SUN50I_CODEC_ANALOG driver selects a nonexisting
symbol SND_SUNXI_ADDA_PR_REGMAP, which was evidently intended to replace
SND_SUN8I_ADDA_PR_REGMAP, but since they are now mismatched, we can run
into link errors for some configurations:
sound/soc/sunxi/sun50i-codec-analog.o: In function `sun50i_codec_analog_probe':
sun50i-codec-analog.c:(.text+0x62): undefined reference to
`sun8i_adda_pr_regmap_init'
The new name appears to be more sensible, and as the symbol is hidden,
there are no downsides in the rename, so use that consistently now.
Fixes: 42371f327df0 ("ASoC: sunxi: Add new driver for Allwinner A64 codec's
analog path controls")
Signed-off-by: Arnd Bergmann
---
sound/soc/sunxi/Kconfig | 4 ++--
sound/soc/sunxi/Makefile | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
index 66aad0d3f9c7..0f992bdf1a10 100644
--- a/sound/soc/sunxi/Kconfig
+++ b/sound/soc/sunxi/Kconfig
@@ -23,7 +23,7 @@ config SND_SUN8I_CODEC
config SND_SUN8I_CODEC_ANALOG
tristate "Allwinner sun8i Codec Analog Controls Support"
depends on MACH_SUN8I || (ARM64 && ARCH_SUNXI) || COMPILE_TEST
- select SND_SUN8I_ADDA_PR_REGMAP
+ select SND_SUNXI_ADDA_PR_REGMAP
help
Say Y or M if you want to add support for the analog controls for
the codec embedded in newer Allwinner SoCs.
@@ -54,7 +54,7 @@ config SND_SUN4I_SPDIF
Say Y or M to add support for the S/PDIF audio block in the Allwinner
A10 and affiliated SoCs.
-config SND_SUN8I_ADDA_PR_REGMAP
+config SND_SUNXI_ADDA_PR_REGMAP
tristate
select REGMAP
diff --git a/sound/soc/sunxi/Makefile b/sound/soc/sunxi/Makefile
index a86be340a076..c4dd2803a011 100644
--- a/sound/soc/sunxi/Makefile
+++ b/sound/soc/sunxi/Makefile
@@ -5,4 +5,4 @@ obj-$(CONFIG_SND_SUN4I_SPDIF) += sun4i-spdif.o
obj-$(CONFIG_SND_SUN8I_CODEC_ANALOG) += sun8i-codec-analog.o
obj-$(CONFIG_SND_SUN50I_CODEC_ANALOG) += sun50i-codec-analog.o
obj-$(CONFIG_SND_SUN8I_CODEC) += sun8i-codec.o
-obj-$(CONFIG_SND_SUN8I_ADDA_PR_REGMAP) += sun8i-adda-pr-regmap.o
+obj-$(CONFIG_SND_SUNXI_ADDA_PR_REGMAP) += sun8i-adda-pr-regmap.o
--
2.18.0
[PATCH] mm: fix uninitialized variable warnings
In a rare randconfig build, I got a warning about possibly uninitialized
variables:
mm/page-writeback.c: In function 'balance_dirty_pages':
mm/page-writeback.c:1623:16: error: 'writeback' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
mdtc->dirty += writeback;
^~
mm/page-writeback.c:1624:4: error: 'filepages' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
mdtc_calc_avail(mdtc, filepages, headroom);
^~
mm/page-writeback.c:1624:4: error: 'headroom' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
The compiler evidently fails to notice that the usage is in dead code
after 'mdtc' is set to NULL when CONFIG_CGROUP_WRITEBACK is disabled.
Adding an IS_ENABLED() check makes this clear to the compiler.
Signed-off-by: Arnd Bergmann
---
mm/page-writeback.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 3f690bae6b78..f02535b7731a 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1611,7 +1611,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
bg_thresh = gdtc->bg_thresh;
}
- if (mdtc) {
+ if (IS_ENABLED(CONFIG_CGROUP_WRITEBACK) && mdtc) {
unsigned long filepages, headroom, writeback;
/*
@@ -1944,7 +1944,7 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
wb_calc_thresh(gdtc->wb, gdtc->bg_thresh))
return true;
- if (mdtc) {
+ if (IS_ENABLED(CONFIG_CGROUP_WRITEBACK) && mdtc) {
unsigned long filepages, headroom, writeback;
mem_cgroup_wb_stats(wb, , , >dirty,
--
2.18.0
[PATCH] EDAC: skx_edac: add ACPI dependency
We cannot currently select ACPI_ADXL without also enabling the top-level
ACPI option:
WARNING: unmet direct dependencies detected for ACPI_ADXL
Depends on [n]: ACPI [=n]
Selected by [y]:
- EDAC_SKX [=y] && EDAC [=y] && PCI [=y] && X86_64 [=y] && X86_MCE_INTEL [=y]
&& PCI_MMCONFIG [=y] && (ACPI_NFIT [=n] || !ACPI_NFIT [=n])
Add a corresponding dependency here.
Fixes: ad6e16059d8e ("EDAC, skx_edac: Add address translation for non-volatile
DIMMs")
Signed-off-by: Arnd Bergmann
---
drivers/edac/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 41c9ccdd20d6..2397e1d2493a 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -231,7 +231,7 @@ config EDAC_SBRIDGE
config EDAC_SKX
tristate "Intel Skylake server Integrated MC"
- depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
+ depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG && ACPI
depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_SKX can't be y
select DMI
select ACPI_ADXL if ACPI
--
2.18.0
[PATCH] arm64: hide unused swiotlb functions
After a good chunk of the swiotlb code has been replaced with the generic
version, two functions are only used from inside of an #ifdef:
arch/arm64/mm/dma-mapping.c:174:12: error: '__swiotlb_mmap_pfn' defined but not
used [-Werror=unused-function]
static int __swiotlb_mmap_pfn(struct vm_area_struct *vma,
^~
arch/arm64/mm/dma-mapping.c:163:12: error: '__swiotlb_get_sgtable_page' defined
but not used [-Werror=unused-function]
static int __swiotlb_get_sgtable_page(struct sg_table *sgt,
Move them into the same #ifdef section to avoid that warning.
Fixes: 886643b76632 ("arm64: use the generic swiotlb_dma_ops")
Signed-off-by: Arnd Bergmann
---
arch/arm64/mm/dma-mapping.c | 58 ++---
1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 3a703e5d4e32..62356c64e180 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -160,35 +160,6 @@ void arch_sync_dma_for_cpu(struct device *dev, phys_addr_t
paddr,
__dma_unmap_area(phys_to_virt(paddr), size, dir);
}
-static int __swiotlb_get_sgtable_page(struct sg_table *sgt,
- struct page *page, size_t size)
-{
- int ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
-
- if (!ret)
- sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
-
- return ret;
-}
-
-static int __swiotlb_mmap_pfn(struct vm_area_struct *vma,
- unsigned long pfn, size_t size)
-{
- int ret = -ENXIO;
- unsigned long nr_vma_pages = vma_pages(vma);
- unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
- unsigned long off = vma->vm_pgoff;
-
- if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
- ret = remap_pfn_range(vma, vma->vm_start,
- pfn + off,
- vma->vm_end - vma->vm_start,
- vma->vm_page_prot);
- }
-
- return ret;
-}
-
static int __init atomic_pool_init(void)
{
pgprot_t prot = __pgprot(PROT_NORMAL_NC);
@@ -358,6 +329,35 @@ arch_initcall(arm64_dma_init);
#include
#include
+static int __swiotlb_get_sgtable_page(struct sg_table *sgt,
+ struct page *page, size_t size)
+{
+ int ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
+
+ if (!ret)
+ sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
+
+ return ret;
+}
+
+static int __swiotlb_mmap_pfn(struct vm_area_struct *vma,
+ unsigned long pfn, size_t size)
+{
+ int ret = -ENXIO;
+ unsigned long nr_vma_pages = vma_pages(vma);
+ unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+ unsigned long off = vma->vm_pgoff;
+
+ if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
+ ret = remap_pfn_range(vma, vma->vm_start,
+ pfn + off,
+ vma->vm_end - vma->vm_start,
+ vma->vm_page_prot);
+ }
+
+ return ret;
+}
+
/* Thankfully, all cache ops are by VA so we can ignore phys here */
static void flush_page(struct device *dev, const void *virt, phys_addr_t phys)
{
--
2.18.0
[PATCH] [v2] ASoC: wm97xx: fix uninitialized regmap pointer problem
gcc notices that without either the ac97 bus or the pdata, we never
initialize the regmap pointer, which leads to an uninitialized variable
access:
sound/soc/codecs/wm9712.c: In function 'wm9712_soc_probe':
sound/soc/codecs/wm9712.c:666:2: error: 'regmap' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
Since that configuration is invalid, it's better to return an error
here. I tried to avoid adding complexity to the conditions, and turned
the #ifdef into a regular if(IS_ENABLED()) check for readability.
This in turn requires moving some header file declarations out of
an #ifdef.
The same code is used in three drivers, all of which I'm changing
the same way.
Fixes: 2ed1a8e0ce8d ("ASoC: wm9712: add ac97 new bus support")
Signed-off-by: Arnd Bergmann
---
v2: fix a missing check that Charles pointed out
---
include/sound/soc.h | 2 +-
sound/soc/codecs/wm9705.c | 10 --
sound/soc/codecs/wm9712.c | 10 --
sound/soc/codecs/wm9713.c | 10 --
4 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 70c10a8f3e90..3e0ac310a3df 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -553,12 +553,12 @@ static inline void snd_soc_jack_free_gpios(struct
snd_soc_jack *jack, int count,
}
#endif
-#ifdef CONFIG_SND_SOC_AC97_BUS
struct snd_ac97 *snd_soc_alloc_ac97_component(struct snd_soc_component
*component);
struct snd_ac97 *snd_soc_new_ac97_component(struct snd_soc_component
*component,
unsigned int id, unsigned int id_mask);
void snd_soc_free_ac97_component(struct snd_ac97 *ac97);
+#ifdef CONFIG_SND_SOC_AC97_BUS
int snd_soc_set_ac97_ops(struct snd_ac97_bus_ops *ops);
int snd_soc_set_ac97_ops_of_reset(struct snd_ac97_bus_ops *ops,
struct platform_device *pdev);
diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
index ccdf088461b7..54c306707c02 100644
--- a/sound/soc/codecs/wm9705.c
+++ b/sound/soc/codecs/wm9705.c
@@ -325,8 +325,7 @@ static int wm9705_soc_probe(struct snd_soc_component
*component)
if (wm9705->mfd_pdata) {
wm9705->ac97 = wm9705->mfd_pdata->ac97;
regmap = wm9705->mfd_pdata->regmap;
- } else {
-#ifdef CONFIG_SND_SOC_AC97_BUS
+ } else if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS)) {
wm9705->ac97 = snd_soc_new_ac97_component(component,
WM9705_VENDOR_ID,
WM9705_VENDOR_ID_MASK);
if (IS_ERR(wm9705->ac97)) {
@@ -339,7 +338,8 @@ static int wm9705_soc_probe(struct snd_soc_component
*component)
snd_soc_free_ac97_component(wm9705->ac97);
return PTR_ERR(regmap);
}
-#endif
+ } else {
+ return -ENXIO;
}
snd_soc_component_set_drvdata(component, wm9705->ac97);
@@ -350,14 +350,12 @@ static int wm9705_soc_probe(struct snd_soc_component
*component)
static void wm9705_soc_remove(struct snd_soc_component *component)
{
-#ifdef CONFIG_SND_SOC_AC97_BUS
struct wm9705_priv *wm9705 = snd_soc_component_get_drvdata(component);
- if (!wm9705->mfd_pdata) {
+ if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS) && !wm9705->mfd_pdata) {
snd_soc_component_exit_regmap(component);
snd_soc_free_ac97_component(wm9705->ac97);
}
-#endif
}
static const struct snd_soc_component_driver soc_component_dev_wm9705 = {
diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index e873baa9e778..01949eaba4fd 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -642,8 +642,7 @@ static int wm9712_soc_probe(struct snd_soc_component
*component)
if (wm9712->mfd_pdata) {
wm9712->ac97 = wm9712->mfd_pdata->ac97;
regmap = wm9712->mfd_pdata->regmap;
- } else {
-#ifdef CONFIG_SND_SOC_AC97_BUS
+ } else if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS)) {
int ret;
wm9712->ac97 = snd_soc_new_ac97_component(component,
WM9712_VENDOR_ID,
@@ -660,7 +659,8 @@ static int wm9712_soc_probe(struct snd_soc_component
*component)
snd_soc_free_ac97_component(wm9712->ac97);
return PTR_ERR(regmap);
}
-#endif
+ } else {
+ return -ENXIO;
}
snd_soc_component_init_regmap(component, regmap);
@@ -673,14 +673,12 @@ static int wm9712_soc_probe(struct snd_soc_component
*component)
static void wm9712_soc_remove(struct snd_soc_component *component)
{
-#ifdef CONFIG_SND_SOC_AC97_BUS
struct wm9712_priv *wm9712 = snd_soc_component_get_drvdata(component);
- if (!wm9712->mfd_pdata) {
+ if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS) && !wm9712->mfd_pdata) {
snd_soc_component_exit_regmap(component);
Re: [PATCH] ASoC: wm97xx: fix uninitialized regmap pointer problem
On 11/2/18, Charles Keepax wrote:
> On Fri, Nov 02, 2018 at 12:23:08PM +0100, Arnd Bergmann wrote:
>> static void wm9713_soc_remove(struct snd_soc_component *component)
>> {
>> -#ifdef CONFIG_SND_SOC_AC97_BUS
>> struct wm9713_priv *wm9713 = snd_soc_component_get_drvdata(component);
>>
>> -if (!wm9713->mfd_pdata) {
>> +if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS)) {
>
> Should this one not also have an && !wm9713->mfd_pdata?
Right, good catch!. I'll send a fixed version in a bit.
Arnd
[PATCH] HID: asus: fix build warning wiht CONFIG_ASUS_WMI disabled
asus_wmi_evaluate_method() is an empty dummy function when CONFIG_ASUS_WMI
is disabled, or not reachable from a built-in device driver. This leads to
a theoretical evaluation of an uninitialized variable that the compiler
complains about, failing to check that the hardcoded return value makes
this an unreachable code path:
In file included from include/linux/printk.h:336,
from include/linux/kernel.h:14,
from include/linux/list.h:9,
from include/linux/dmi.h:5,
from drivers/hid/hid-asus.c:29:
drivers/hid/hid-asus.c: In function 'asus_input_configured':
include/linux/dynamic_debug.h:135:3: error: 'value' may be used uninitialized
in this function [-Werror=maybe-uninitialized]
__dynamic_dev_dbg(, dev, fmt, \
^
drivers/hid/hid-asus.c:359:6: note: 'value' was declared here
u32 value;
^
With an extra IS_ENABLED() check, the warning goes away.
Fixes: 3b692c55e58d ("HID: asus: only support backlight when it's not driven by
WMI")
Signed-off-by: Arnd Bergmann
---
drivers/hid/hid-asus.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index dc6d6477e961..a1fa2fc8c9b5 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -359,6 +359,9 @@ static bool asus_kbd_wmi_led_control_present(struct
hid_device *hdev)
u32 value;
int ret;
+ if (!IS_ENABLED(CONFIG_ASUS_WMI))
+ return false;
+
ret = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS2,
ASUS_WMI_DEVID_KBD_BACKLIGHT, 0, );
hid_dbg(hdev, "WMI backlight check: rc %d value %x", ret, value);
--
2.18.0
[PATCH] ASoC: skl: always select SND_SOC_HDAC_HDA
The skylake sound support is written to work both with or without CONFIG_SND_SOC_HDAC_HDA, and uses an #ifdef to decide whether it should link against that. However, this fails with SND_SOC_ALL_CODECS=m or SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=m when the Skylake support itself is built-in, with this link error: sound/soc/intel/skylake/skl.o: In function `skl_probe': skl.c:(.text+0x56c): undefined reference to `snd_soc_hdac_hda_get_ops' Using an explicit 'select' here simplifies the logic and prevents it from happening, at the cost of always including the compile time dependency. Signed-off-by: Arnd Bergmann --- sound/soc/intel/Kconfig | 1 + sound/soc/intel/skylake/skl.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 0caa1f4eb94d..c21ce7624af1 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -109,6 +109,7 @@ config SND_SOC_INTEL_SKYLAKE depends on PCI && ACPI select SND_HDA_EXT_CORE select SND_HDA_DSP_LOADER + select SND_SOC_HDAC_HDA select SND_SOC_TOPOLOGY select SND_SOC_INTEL_SST select SND_SOC_ACPI_INTEL_MATCH diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 29225623b4b4..1069ee265ce5 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -870,9 +870,7 @@ static int skl_create(struct pci_dev *pci, hbus = skl_to_hbus(skl); bus = skl_to_bus(skl); -#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA) ext_ops = snd_soc_hdac_hda_get_ops(); -#endif snd_hdac_ext_bus_init(bus, >dev, _core_ops, io_ops, ext_ops); bus->use_posbuf = 1; skl->pci = pci; -- 2.18.0
[PATCH] ASoC: wm97xx: fix uninitialized regmap pointer problem
gcc notices that without either the ac97 bus or the pdata, we never
initialize the regmap pointer, which leads to an uninitialized variable
access:
sound/soc/codecs/wm9712.c: In function 'wm9712_soc_probe':
sound/soc/codecs/wm9712.c:666:2: error: 'regmap' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
Since that configuration is invalid, it's better to return an error
here. I tried to avoid adding complexity to the conditions, and turned
the #ifdef into a regular if(IS_ENABLED()) check for readability.
This in turn requires moving some header file declarations out of
an #ifdef.
The same code is used in three drivers, all of which I'm changing
the same way.
Fixes: 2ed1a8e0ce8d ("ASoC: wm9712: add ac97 new bus support")
Signed-off-by: Arnd Bergmann
---
include/sound/soc.h | 2 +-
sound/soc/codecs/wm9705.c | 10 --
sound/soc/codecs/wm9712.c | 10 --
sound/soc/codecs/wm9713.c | 10 --
4 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 70c10a8f3e90..3e0ac310a3df 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -553,12 +553,12 @@ static inline void snd_soc_jack_free_gpios(struct
snd_soc_jack *jack, int count,
}
#endif
-#ifdef CONFIG_SND_SOC_AC97_BUS
struct snd_ac97 *snd_soc_alloc_ac97_component(struct snd_soc_component
*component);
struct snd_ac97 *snd_soc_new_ac97_component(struct snd_soc_component
*component,
unsigned int id, unsigned int id_mask);
void snd_soc_free_ac97_component(struct snd_ac97 *ac97);
+#ifdef CONFIG_SND_SOC_AC97_BUS
int snd_soc_set_ac97_ops(struct snd_ac97_bus_ops *ops);
int snd_soc_set_ac97_ops_of_reset(struct snd_ac97_bus_ops *ops,
struct platform_device *pdev);
diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
index ccdf088461b7..54c306707c02 100644
--- a/sound/soc/codecs/wm9705.c
+++ b/sound/soc/codecs/wm9705.c
@@ -325,8 +325,7 @@ static int wm9705_soc_probe(struct snd_soc_component
*component)
if (wm9705->mfd_pdata) {
wm9705->ac97 = wm9705->mfd_pdata->ac97;
regmap = wm9705->mfd_pdata->regmap;
- } else {
-#ifdef CONFIG_SND_SOC_AC97_BUS
+ } else if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS)) {
wm9705->ac97 = snd_soc_new_ac97_component(component,
WM9705_VENDOR_ID,
WM9705_VENDOR_ID_MASK);
if (IS_ERR(wm9705->ac97)) {
@@ -339,7 +338,8 @@ static int wm9705_soc_probe(struct snd_soc_component
*component)
snd_soc_free_ac97_component(wm9705->ac97);
return PTR_ERR(regmap);
}
-#endif
+ } else {
+ return -ENXIO;
}
snd_soc_component_set_drvdata(component, wm9705->ac97);
@@ -350,14 +350,12 @@ static int wm9705_soc_probe(struct snd_soc_component
*component)
static void wm9705_soc_remove(struct snd_soc_component *component)
{
-#ifdef CONFIG_SND_SOC_AC97_BUS
struct wm9705_priv *wm9705 = snd_soc_component_get_drvdata(component);
- if (!wm9705->mfd_pdata) {
+ if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS) && !wm9705->mfd_pdata) {
snd_soc_component_exit_regmap(component);
snd_soc_free_ac97_component(wm9705->ac97);
}
-#endif
}
static const struct snd_soc_component_driver soc_component_dev_wm9705 = {
diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index e873baa9e778..01949eaba4fd 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -642,8 +642,7 @@ static int wm9712_soc_probe(struct snd_soc_component
*component)
if (wm9712->mfd_pdata) {
wm9712->ac97 = wm9712->mfd_pdata->ac97;
regmap = wm9712->mfd_pdata->regmap;
- } else {
-#ifdef CONFIG_SND_SOC_AC97_BUS
+ } else if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS)) {
int ret;
wm9712->ac97 = snd_soc_new_ac97_component(component,
WM9712_VENDOR_ID,
@@ -660,7 +659,8 @@ static int wm9712_soc_probe(struct snd_soc_component
*component)
snd_soc_free_ac97_component(wm9712->ac97);
return PTR_ERR(regmap);
}
-#endif
+ } else {
+ return -ENXIO;
}
snd_soc_component_init_regmap(component, regmap);
@@ -673,14 +673,12 @@ static int wm9712_soc_probe(struct snd_soc_component
*component)
static void wm9712_soc_remove(struct snd_soc_component *component)
{
-#ifdef CONFIG_SND_SOC_AC97_BUS
struct wm9712_priv *wm9712 = snd_soc_component_get_drvdata(component);
- if (!wm9712->mfd_pdata) {
+ if (IS_ENABLED(CONFIG_SND_SOC_AC97_BUS) && !wm9712->mfd_pdata) {
snd_soc_component_exit_regmap(component);
snd_soc_free_ac9
[PATCH] ASoC: pxa: change ac97 dependencies
Enabling both the old AC97_BUS code and the new AC97_BUS_COMPAT causes
problems because both modules provide an exported snd_ac97_reset()
function.
I had tried to fix the problem of having both coexist earlier, but
my patch only prevented them from being built-in. This is because
of a special Kconfig feature that lets a symbol have a dependency
on another one being disabled, but still allow both to be loadable
modules.
Changing the dependency to =n avoids that problem, now we can only
build the new driver if the old one is completely disabled.
If we could figure out a way to let rename one of the reset
functions and have each driver link to exactly the old or
the compat code, that would also work, but I could not find if
that's possible.
Fixes: bec5ecdf41d4 ("ASoC: pxa: avoid AC97_BUS build warning")
Signed-off-by: Arnd Bergmann
---
sound/soc/pxa/Kconfig | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/sound/soc/pxa/Kconfig b/sound/soc/pxa/Kconfig
index 943b44de1464..6075fc32e6b9 100644
--- a/sound/soc/pxa/Kconfig
+++ b/sound/soc/pxa/Kconfig
@@ -79,7 +79,7 @@ config SND_PXA2XX_SOC_TOSA
tristate "SoC AC97 Audio support for Tosa"
depends on SND_PXA2XX_SOC && MACH_TOSA
depends on MFD_TC6393XB
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_PXA2XX_SOC_AC97
select SND_SOC_WM9712
help
@@ -89,7 +89,7 @@ config SND_PXA2XX_SOC_TOSA
config SND_PXA2XX_SOC_E740
tristate "SoC AC97 Audio support for e740"
depends on SND_PXA2XX_SOC && MACH_E740
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_SOC_WM9705
select SND_PXA2XX_SOC_AC97
help
@@ -99,7 +99,7 @@ config SND_PXA2XX_SOC_E740
config SND_PXA2XX_SOC_E750
tristate "SoC AC97 Audio support for e750"
depends on SND_PXA2XX_SOC && MACH_E750
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_SOC_WM9705
select SND_PXA2XX_SOC_AC97
help
@@ -109,7 +109,7 @@ config SND_PXA2XX_SOC_E750
config SND_PXA2XX_SOC_E800
tristate "SoC AC97 Audio support for e800"
depends on SND_PXA2XX_SOC && MACH_E800
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_SOC_WM9712
select SND_PXA2XX_SOC_AC97
help
@@ -120,7 +120,7 @@ config SND_PXA2XX_SOC_EM_X270
tristate "SoC Audio support for CompuLab EM-x270, eXeda and CM-X300"
depends on SND_PXA2XX_SOC && (MACH_EM_X270 || MACH_EXEDA || \
MACH_CM_X300)
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_PXA2XX_SOC_AC97
select SND_SOC_WM9712
help
@@ -131,7 +131,7 @@ config SND_PXA2XX_SOC_PALM27X
bool "SoC Audio support for Palm T|X, T5, E2 and LifeDrive"
depends on SND_PXA2XX_SOC && (MACH_PALMLD || MACH_PALMTX || \
MACH_PALMT5 || MACH_PALMTE2)
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_PXA2XX_SOC_AC97
select SND_SOC_WM9712
help
@@ -161,7 +161,7 @@ config SND_SOC_TTC_DKB
config SND_SOC_ZYLONITE
tristate "SoC Audio support for Marvell Zylonite"
depends on SND_PXA2XX_SOC && MACH_ZYLONITE
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_PXA2XX_SOC_AC97
select SND_PXA_SOC_SSP
select SND_SOC_WM9713
@@ -201,7 +201,7 @@ config SND_PXA2XX_SOC_MAGICIAN
config SND_PXA2XX_SOC_MIOA701
tristate "SoC Audio support for MIO A701"
depends on SND_PXA2XX_SOC && MACH_MIOA701
- depends on !AC97_BUS
+ depends on AC97_BUS=n
select SND_PXA2XX_SOC_AC97
select SND_SOC_WM9713
help
--
2.18.0
[PATCH] ubifs: auth: add CONFIG_KEYS dependency
The new authentication support causes a build failure
when CONFIG_KEYS is disabled, so add a dependency.
fs/ubifs/auth.c: In function 'ubifs_init_authentication':
fs/ubifs/auth.c:249:16: error: implicit declaration of function 'request_key';
did you mean 'request_irq'? [-Werror=implicit-function-declaration]
keyring_key = request_key(_type_logon, c->auth_key_name, NULL);
Fixes: d8a22773a12c ("ubifs: Enable authentication support")
Signed-off-by: Arnd Bergmann
---
fs/ubifs/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
index 529856fbccd0..406c88dc039e 100644
--- a/fs/ubifs/Kconfig
+++ b/fs/ubifs/Kconfig
@@ -89,6 +89,7 @@ config UBIFS_FS_SECURITY
config UBIFS_FS_AUTHENTICATION
bool "UBIFS authentication support"
+ depends on KEYS
select CRYPTO_HMAC
help
Enable authentication support for UBIFS. This feature offers
protection
--
2.18.0
[PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency
We still get a link failure with IOSF_MBI=m when the xpower driver
is built-in:
drivers/acpi/pmic/intel_pmic_xpower.o: In function
`intel_xpower_pmic_update_power':
intel_pmic_xpower.c:(.text+0x4f2): undefined reference to
`iosf_mbi_block_punit_i2c_access'
intel_pmic_xpower.c:(.text+0x5e2): undefined reference to
`iosf_mbi_unblock_punit_i2c_access'
This makes the dependency stronger, so we can only build when IOSF_MBI
is built-in.
Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to Kconfig
entry")
Signed-off-by: Arnd Bergmann
---
drivers/acpi/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 18851e7eedd5..31a3c4a03f61 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION
config XPOWER_PMIC_OPREGION
bool "ACPI operation region support for XPower AXP288 PMIC"
- depends on MFD_AXP20X_I2C && IOSF_MBI
+ depends on MFD_AXP20X_I2C && IOSF_MBI=y
help
This config adds ACPI operation region support for XPower AXP288 PMIC.
--
2.18.0
