Re: [PATCH] riscv: Fix early ftrace nop patching
On Tue, Jun 18, 2024 at 9:40 PM Alexandre Ghiti wrote:
>
> Hi Andy,
>
> On Tue, Jun 18, 2024 at 2:48 PM Andy Chiu wrote:
> >
> > On Tue, Jun 18, 2024 at 8:02 PM Alexandre Ghiti wrote:
> > >
> > > Hi Conor,
> > >
> > > On 17/06/2024 15:23, Alexandre Ghiti wrote:
> > > > Hi Conor,
> > > >
> > > > Sorry for the delay here.
> > > >
> > > > On 13/06/2024 09:48, Conor Dooley wrote:
> > > >> On Thu, May 23, 2024 at 01:51:34PM +0200, Alexandre Ghiti wrote:
> > > >>> Commit c97bf629963e ("riscv: Fix text patching when IPI are used")
> > > >>> converted ftrace_make_nop() to use patch_insn_write() which does not
> > > >>> emit any icache flush relying entirely on __ftrace_modify_code() to do
> > > >>> that.
> > > >>>
> > > >>> But we missed that ftrace_make_nop() was called very early directly
> > > >>> when
> > > >>> converting mcount calls into nops (actually on riscv it converts 2B
> > > >>> nops
> > > >>> emitted by the compiler into 4B nops).
> > > >>>
> > > >>> This caused crashes on multiple HW as reported by Conor and Björn
> > > >>> since
> > > >>> the booting core could have half-patched instructions in its icache
> > > >>> which would trigger an illegal instruction trap: fix this by emitting
> > > >>> a
> > > >>> local flush icache when early patching nops.
> > > >>>
> > > >>> Fixes: c97bf629963e ("riscv: Fix text patching when IPI are used")
> > > >>> Signed-off-by: Alexandre Ghiti
> > > >>> ---
> > > >>> arch/riscv/include/asm/cacheflush.h | 6 ++
> > > >>> arch/riscv/kernel/ftrace.c | 3 +++
> > > >>> 2 files changed, 9 insertions(+)
> > > >>>
> > > >>> diff --git a/arch/riscv/include/asm/cacheflush.h
> > > >>> b/arch/riscv/include/asm/cacheflush.h
> > > >>> index dd8d07146116..ce79c558a4c8 100644
> > > >>> --- a/arch/riscv/include/asm/cacheflush.h
> > > >>> +++ b/arch/riscv/include/asm/cacheflush.h
> > > >>> @@ -13,6 +13,12 @@ static inline void local_flush_icache_all(void)
> > > >>> asm volatile ("fence.i" ::: "memory");
> > > >>> }
> > > >>> +static inline void local_flush_icache_range(unsigned long start,
> > > >>> +unsigned long end)
> > > >>> +{
> > > >>> +local_flush_icache_all();
> > > >>> +}
> > > >>> +
> > > >>> #define PG_dcache_clean PG_arch_1
> > > >>> static inline void flush_dcache_folio(struct folio *folio)
> > > >>> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> > > >>> index 4f4987a6d83d..32e7c401dfb4 100644
> > > >>> --- a/arch/riscv/kernel/ftrace.c
> > > >>> +++ b/arch/riscv/kernel/ftrace.c
> > > >>> @@ -120,6 +120,9 @@ int ftrace_init_nop(struct module *mod, struct
> > > >>> dyn_ftrace *rec)
> > > >>> out = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > > >>> mutex_unlock(_mutex);
> > > >> So, turns out that this patch is not sufficient. I've seen some more
> > > >> crashes, seemingly due to initialising nops on this mutex_unlock().
> > > >> Palmer suggested moving the if (!mod) ... into the lock, which fixed
> > > >> the problem for me.
> > > >
> > > >
> > > > Ok, it makes sense, I completely missed that. I'll send a fix for that
> > > > shortly so that it can be merged in rc5.
> > > >
> > > > Thanks,
> > > >
> > > > Alex
> > >
> > >
> > > So I digged a bit more and I'm afraid that the only way to make sure
> > > this issue does not happen elsewhere is to flush the icache right after
> > > the patching. We actually can't wait to batch the icache flush since
> > > along the way, we may call a function that has just been patched (the
> > > issue that you encountered here).
> >
> > Trying to provide my thoughts, please let me know if I missed
> > anything. I think what Conor suggested is safe for init_nop, as
Re: [PATCH] riscv: Fix early ftrace nop patching
On Tue, Jun 18, 2024 at 8:02 PM Alexandre Ghiti wrote:
>
> Hi Conor,
>
> On 17/06/2024 15:23, Alexandre Ghiti wrote:
> > Hi Conor,
> >
> > Sorry for the delay here.
> >
> > On 13/06/2024 09:48, Conor Dooley wrote:
> >> On Thu, May 23, 2024 at 01:51:34PM +0200, Alexandre Ghiti wrote:
> >>> Commit c97bf629963e ("riscv: Fix text patching when IPI are used")
> >>> converted ftrace_make_nop() to use patch_insn_write() which does not
> >>> emit any icache flush relying entirely on __ftrace_modify_code() to do
> >>> that.
> >>>
> >>> But we missed that ftrace_make_nop() was called very early directly
> >>> when
> >>> converting mcount calls into nops (actually on riscv it converts 2B
> >>> nops
> >>> emitted by the compiler into 4B nops).
> >>>
> >>> This caused crashes on multiple HW as reported by Conor and Björn since
> >>> the booting core could have half-patched instructions in its icache
> >>> which would trigger an illegal instruction trap: fix this by emitting a
> >>> local flush icache when early patching nops.
> >>>
> >>> Fixes: c97bf629963e ("riscv: Fix text patching when IPI are used")
> >>> Signed-off-by: Alexandre Ghiti
> >>> ---
> >>> arch/riscv/include/asm/cacheflush.h | 6 ++
> >>> arch/riscv/kernel/ftrace.c | 3 +++
> >>> 2 files changed, 9 insertions(+)
> >>>
> >>> diff --git a/arch/riscv/include/asm/cacheflush.h
> >>> b/arch/riscv/include/asm/cacheflush.h
> >>> index dd8d07146116..ce79c558a4c8 100644
> >>> --- a/arch/riscv/include/asm/cacheflush.h
> >>> +++ b/arch/riscv/include/asm/cacheflush.h
> >>> @@ -13,6 +13,12 @@ static inline void local_flush_icache_all(void)
> >>> asm volatile ("fence.i" ::: "memory");
> >>> }
> >>> +static inline void local_flush_icache_range(unsigned long start,
> >>> +unsigned long end)
> >>> +{
> >>> +local_flush_icache_all();
> >>> +}
> >>> +
> >>> #define PG_dcache_clean PG_arch_1
> >>> static inline void flush_dcache_folio(struct folio *folio)
> >>> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> >>> index 4f4987a6d83d..32e7c401dfb4 100644
> >>> --- a/arch/riscv/kernel/ftrace.c
> >>> +++ b/arch/riscv/kernel/ftrace.c
> >>> @@ -120,6 +120,9 @@ int ftrace_init_nop(struct module *mod, struct
> >>> dyn_ftrace *rec)
> >>> out = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> >>> mutex_unlock(_mutex);
> >> So, turns out that this patch is not sufficient. I've seen some more
> >> crashes, seemingly due to initialising nops on this mutex_unlock().
> >> Palmer suggested moving the if (!mod) ... into the lock, which fixed
> >> the problem for me.
> >
> >
> > Ok, it makes sense, I completely missed that. I'll send a fix for that
> > shortly so that it can be merged in rc5.
> >
> > Thanks,
> >
> > Alex
>
>
> So I digged a bit more and I'm afraid that the only way to make sure
> this issue does not happen elsewhere is to flush the icache right after
> the patching. We actually can't wait to batch the icache flush since
> along the way, we may call a function that has just been patched (the
> issue that you encountered here).
Trying to provide my thoughts, please let me know if I missed
anything. I think what Conor suggested is safe for init_nop, as it
would be called only when there is only one core (booting) and at the
loading stage of kernel modules. In the first case we just have to
make sure there is no patchable entry before the core executes
fence.i. The second case is unconditionally safe because there is no
read-side of the race.
It is a bit tricky for the generic (runtime) case of ftrace code
patching, but that is not because of the batch fence.i maintenance. As
long as there exists a patchable entry for the stopping thread, it is
possible for them to execute on a partially patched instruction. A
solution for this is again to prevent any patchable entry in the
stop_machine's stopping thread. Another solution is to apply the
atomic ftrace patching series which aims to get rid of the race.
>
> I don't know how much it will impact the performance but I guess it will.
>
> Unless someone has a better idea (I added Andy and Puranjay in cc), here
> is the patch that implements this, ca
Re: [PATCH 4/8] riscv: ftrace: align patchable functions to 4 Byte boundary
Sorry for the noise, On Mon, Jun 17, 2024 at 10:38 AM Andy Chiu wrote: > > On Fri, Jun 14, 2024 at 3:09 AM Nathan Chancellor wrote: > > > > Hi Andy, > > > > On Thu, Jun 13, 2024 at 03:11:09PM +0800, Andy Chiu wrote: > > > We are changing ftrace code patching in order to remove dependency from > > > stop_machine() and enable kernel preemption. This requires us to align > > > functions entry at a 4-B align address. > > > > > > However, -falign-functions on older versions of GCC alone was not strong > > > enoungh to align all functions. In fact, cold functions are not aligned > > > after turning on optimizations. We consider this is a bug in GCC and > > > turn off guess-branch-probility as a workaround to align all functions. > > > > > > GCC bug id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345 > > > > > > The option -fmin-function-alignment is able to align all functions > > > properly on newer versions of gcc. So, we add a cc-option to test if > > > the toolchain supports it. > > > > > > Suggested-by: Evgenii Shatokhin > > > Signed-off-by: Andy Chiu > > > --- > > > arch/riscv/Kconfig | 1 + > > > arch/riscv/Makefile | 7 ++- > > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > > index b94176e25be1..80b8d48e1e46 100644 > > > --- a/arch/riscv/Kconfig > > > +++ b/arch/riscv/Kconfig > > > @@ -203,6 +203,7 @@ config CLANG_SUPPORTS_DYNAMIC_FTRACE > > > config GCC_SUPPORTS_DYNAMIC_FTRACE > > > def_bool CC_IS_GCC > > > depends on $(cc-option,-fpatchable-function-entry=8) > > > + depends on $(cc-option,-fmin-function-alignment=4) || !RISCV_ISA_C > > > > Please use CC_HAS_MIN_FUNCTION_ALIGNMENT (from arch/Kconfig), which > > already checks for support for this option. > > Thanks for the suggestion! > > > > > > config HAVE_SHADOW_CALL_STACK > > > def_bool $(cc-option,-fsanitize=shadow-call-stack) > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile > > > index 06de9d365088..74628ad8dcf8 100644 > > > --- a/arch/riscv/Makefile > > > +++ b/arch/riscv/Makefile > > > @@ -14,8 +14,13 @@ endif > > > ifeq ($(CONFIG_DYNAMIC_FTRACE),y) > > > LDFLAGS_vmlinux += --no-relax > > > KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY > > > +ifeq ($(CONFIG_CC_IS_CLANG),y) > > > > Same here, please invert this and use > > > > ifdef CONFIG_CC_HAS_MIN_FUNCTION_ALIGNMENT > > > > like the main Makefile does. > > Hope this makes sense to you. I am going to add the following in riscv Kconig: > > select FUNCTION_ALIGNMENT_4B if DYNAMIC_FTRACE && !RISCV_ISA_C This should be: select FUNCTION_ALIGNMENT_4B if DYNAMIC_FTRACE && RISCV_ISA_C as RISCV_ISA_C == y means that there are 2B instructions. In this case, functions can be non 4B aligned, so we need to enforce the alignment requirement from the compiler. > > So we will not need any of these > > > > > > + cflags_ftrace_align := -falign-functions=4 > > > +else > > > + cflags_ftrace_align := -fmin-function-alignment=4 > > > +endif > > > ifeq ($(CONFIG_RISCV_ISA_C),y) > > > - CC_FLAGS_FTRACE := -fpatchable-function-entry=4 > > > + CC_FLAGS_FTRACE := -fpatchable-function-entry=4 > > > $(cflags_ftrace_align) > > > else > > > CC_FLAGS_FTRACE := -fpatchable-function-entry=2 > > > endif > > > > > > -- > > > 2.43.0 > > > > > > > > Thanks, > Andy
Re: [PATCH 4/8] riscv: ftrace: align patchable functions to 4 Byte boundary
On Fri, Jun 14, 2024 at 3:09 AM Nathan Chancellor wrote: > > Hi Andy, > > On Thu, Jun 13, 2024 at 03:11:09PM +0800, Andy Chiu wrote: > > We are changing ftrace code patching in order to remove dependency from > > stop_machine() and enable kernel preemption. This requires us to align > > functions entry at a 4-B align address. > > > > However, -falign-functions on older versions of GCC alone was not strong > > enoungh to align all functions. In fact, cold functions are not aligned > > after turning on optimizations. We consider this is a bug in GCC and > > turn off guess-branch-probility as a workaround to align all functions. > > > > GCC bug id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345 > > > > The option -fmin-function-alignment is able to align all functions > > properly on newer versions of gcc. So, we add a cc-option to test if > > the toolchain supports it. > > > > Suggested-by: Evgenii Shatokhin > > Signed-off-by: Andy Chiu > > --- > > arch/riscv/Kconfig | 1 + > > arch/riscv/Makefile | 7 ++- > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index b94176e25be1..80b8d48e1e46 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -203,6 +203,7 @@ config CLANG_SUPPORTS_DYNAMIC_FTRACE > > config GCC_SUPPORTS_DYNAMIC_FTRACE > > def_bool CC_IS_GCC > > depends on $(cc-option,-fpatchable-function-entry=8) > > + depends on $(cc-option,-fmin-function-alignment=4) || !RISCV_ISA_C > > Please use CC_HAS_MIN_FUNCTION_ALIGNMENT (from arch/Kconfig), which > already checks for support for this option. Thanks for the suggestion! > > > config HAVE_SHADOW_CALL_STACK > > def_bool $(cc-option,-fsanitize=shadow-call-stack) > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile > > index 06de9d365088..74628ad8dcf8 100644 > > --- a/arch/riscv/Makefile > > +++ b/arch/riscv/Makefile > > @@ -14,8 +14,13 @@ endif > > ifeq ($(CONFIG_DYNAMIC_FTRACE),y) > > LDFLAGS_vmlinux += --no-relax > > KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY > > +ifeq ($(CONFIG_CC_IS_CLANG),y) > > Same here, please invert this and use > > ifdef CONFIG_CC_HAS_MIN_FUNCTION_ALIGNMENT > > like the main Makefile does. Hope this makes sense to you. I am going to add the following in riscv Kconig: select FUNCTION_ALIGNMENT_4B if DYNAMIC_FTRACE && !RISCV_ISA_C So we will not need any of these > > > + cflags_ftrace_align := -falign-functions=4 > > +else > > + cflags_ftrace_align := -fmin-function-alignment=4 > > +endif > > ifeq ($(CONFIG_RISCV_ISA_C),y) > > - CC_FLAGS_FTRACE := -fpatchable-function-entry=4 > > + CC_FLAGS_FTRACE := -fpatchable-function-entry=4 $(cflags_ftrace_align) > > else > > CC_FLAGS_FTRACE := -fpatchable-function-entry=2 > > endif > > > > -- > > 2.43.0 > > > > Thanks, Andy
Re: [PATCH 2/8] tracing: do not trace kernel_text_address()
On Thu, Jun 13, 2024 at 9:32 PM Steven Rostedt wrote: > > On Thu, 13 Jun 2024 15:11:07 +0800 > Andy Chiu wrote: > > > kernel_text_address() and __kernel_text_address() are called in > > arch_stack_walk() of riscv. This results in excess amount of un-related > > traces when the kernel is compiled with CONFIG_TRACE_IRQFLAGS. The > > situation worsens when function_graph is active, as it calls > > local_irq_save/restore in each function's entry/exit. This patch adds > > both functions to notrace, so they won't show up on the trace records. > > I rather not add notrace just because something is noisy. > > You can always just add: > > echo '*kernel_text_address' > /sys/kernel/tracing/set_ftrace_notrace > > and achieve the same result. Sounds good, I am going to drop this patch for the next revision > > -- Steve Thanks, Andy
[PATCH 8/8] riscv: ftrace: support PREEMPT
Now, we can safely enable dynamic ftrace with kernel preemption. Signed-off-by: Andy Chiu --- arch/riscv/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 80b8d48e1e46..c1493ee1b8cd 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -133,7 +133,7 @@ config RISCV select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL select HAVE_FUNCTION_GRAPH_TRACER select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER - select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION + select HAVE_FUNCTION_TRACER if !XIP_KERNEL select HAVE_EBPF_JIT if MMU select HAVE_GUP_FAST if MMU select HAVE_FUNCTION_ARG_ACCESS_API -- 2.43.0
[PATCH 7/8] riscv: vector: Support calling schedule() for preemptible Vector
Each function entry implies a call to ftrace infrastructure. And it may
call into schedule in some cases. So, it is possible for preemptible
kernel-mode Vector to implicitly call into schedule. Since all V-regs
are caller-saved, it is possible to drop all V context when a thread
voluntarily call schedule(). Besides, we currently don't pass argument
through vector register, so we don't have to save/restore V-regs in
ftrace trampoline.
Signed-off-by: Andy Chiu
---
arch/riscv/include/asm/processor.h | 5 +
arch/riscv/include/asm/vector.h| 22 +++---
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/riscv/include/asm/processor.h
b/arch/riscv/include/asm/processor.h
index 68c3432dc6ea..02598e168659 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -95,6 +95,10 @@ struct pt_regs;
* Thus, the task does not own preempt_v. Any use of Vector will have to
* save preempt_v, if dirty, and fallback to non-preemptible kernel-mode
* Vector.
+ * - bit 29: The thread voluntarily calls schedule() while holding an active
+ *preempt_v. All preempt_v context should be dropped in such case because
+ *V-regs are caller-saved. Only sstatus.VS=ON is persisted across a
+ *schedule() call.
* - bit 30: The in-kernel preempt_v context is saved, and requries to be
*restored when returning to the context that owns the preempt_v.
* - bit 31: The in-kernel preempt_v context is dirty, as signaled by the
@@ -109,6 +113,7 @@ struct pt_regs;
#define RISCV_PREEMPT_V0x0100
#define RISCV_PREEMPT_V_DIRTY 0x8000
#define RISCV_PREEMPT_V_NEED_RESTORE 0x4000
+#define RISCV_PREEMPT_V_IN_SCHEDULE0x2000
/* CPU-specific state of a task */
struct thread_struct {
diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index 731dcd0ed4de..50693cffbe78 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -75,6 +75,11 @@ static __always_inline void riscv_v_disable(void)
csr_clear(CSR_SSTATUS, SR_VS);
}
+static __always_inline bool riscv_v_is_on(void)
+{
+ return !!(csr_read(CSR_SSTATUS) & SR_VS);
+}
+
static __always_inline void __vstate_csr_save(struct __riscv_v_ext_state *dest)
{
asm volatile (
@@ -243,6 +248,11 @@ static inline void __switch_to_vector(struct task_struct
*prev,
struct pt_regs *regs;
if (riscv_preempt_v_started(prev)) {
+ if (riscv_v_is_on()) {
+ WARN_ON(prev->thread.riscv_v_flags &
RISCV_V_CTX_DEPTH_MASK);
+ riscv_v_disable();
+ prev->thread.riscv_v_flags |=
RISCV_PREEMPT_V_IN_SCHEDULE;
+ }
if (riscv_preempt_v_dirty(prev)) {
__riscv_v_vstate_save(>thread.kernel_vstate,
prev->thread.kernel_vstate.datap);
@@ -253,10 +263,16 @@ static inline void __switch_to_vector(struct task_struct
*prev,
riscv_v_vstate_save(>thread.vstate, regs);
}
- if (riscv_preempt_v_started(next))
- riscv_preempt_v_set_restore(next);
- else
+ if (riscv_preempt_v_started(next)) {
+ if (next->thread.riscv_v_flags & RISCV_PREEMPT_V_IN_SCHEDULE) {
+ next->thread.riscv_v_flags &=
~RISCV_PREEMPT_V_IN_SCHEDULE;
+ riscv_v_enable();
+ } else {
+ riscv_preempt_v_set_restore(next);
+ }
+ } else {
riscv_v_vstate_set_restore(next, task_pt_regs(next));
+ }
}
void riscv_v_vstate_ctrl_init(struct task_struct *tsk);
--
2.43.0
[PATCH 6/8] riscv: ftrace: do not use stop_machine to update code
Now it is safe to remove dependency from stop_machine() for us to patch
code in ftrace.
Signed-off-by: Andy Chiu
---
arch/riscv/kernel/ftrace.c | 53 --
1 file changed, 4 insertions(+), 49 deletions(-)
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index f3b09f2d3ecc..9a421e151b1d 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -13,23 +13,13 @@
#include
#ifdef CONFIG_DYNAMIC_FTRACE
-void ftrace_arch_code_modify_prepare(void) __acquires(_mutex)
+void arch_ftrace_update_code(int command)
{
mutex_lock(_mutex);
-
- /*
-* The code sequences we use for ftrace can't be patched while the
-* kernel is running, so we need to use stop_machine() to modify them
-* for now. This doesn't play nice with text_mutex, we use this flag
-* to elide the check.
-*/
- riscv_patch_in_stop_machine = true;
-}
-
-void ftrace_arch_code_modify_post_process(void) __releases(_mutex)
-{
- riscv_patch_in_stop_machine = false;
+ command |= FTRACE_MAY_SLEEP;
+ ftrace_modify_all_code(command);
mutex_unlock(_mutex);
+ flush_icache_all();
}
static int ftrace_check_current_call(unsigned long hook_pos,
@@ -158,41 +148,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
return __ftrace_modify_call_site(_call_dest, func, true);
}
-struct ftrace_modify_param {
- int command;
- atomic_t cpu_count;
-};
-
-static int __ftrace_modify_code(void *data)
-{
- struct ftrace_modify_param *param = data;
-
- if (atomic_inc_return(>cpu_count) == num_online_cpus()) {
- ftrace_modify_all_code(param->command);
- /*
-* Make sure the patching store is effective *before* we
-* increment the counter which releases all waiting CPUs
-* by using the release variant of atomic increment. The
-* release pairs with the call to local_flush_icache_all()
-* on the waiting CPU.
-*/
- atomic_inc_return_release(>cpu_count);
- } else {
- while (atomic_read(>cpu_count) <= num_online_cpus())
- cpu_relax();
- }
-
- local_flush_icache_all();
-
- return 0;
-}
-
-void arch_ftrace_update_code(int command)
-{
- struct ftrace_modify_param param = { command, ATOMIC_INIT(0) };
-
- stop_machine(__ftrace_modify_code, , cpu_online_mask);
-}
#endif
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
--
2.43.0
[PATCH 5/8] riscv: ftrace: prepare ftrace for atomic code patching
We use an AUIPC+JALR pair to jump into a ftrace trampoline. Since
instruction fetch can break down to 4 byte at a time, it is impossible
to update two instructions without a race. In order to mitigate it, we
initialize the patchable entry to AUIPC + NOP4. Then, the run-time code
patching can change NOP4 to JALR to eable/disable ftrcae from a
function. This limits the reach of each ftrace entry to +-2KB displacing
from ftrace_caller.
Starting from the trampoline, we add a level of indirection for it to
reach ftrace caller target. Now, it loads the target address from a
memory location, then perform the jump. This enable the kernel to update
the target atomically.
The ordering of reading/updating the targert address should be guarded
by generic ftrace code, where it sends smp_rmb ipi.
Signed-off-by: Andy Chiu
---
arch/riscv/include/asm/ftrace.h | 4 +++
arch/riscv/kernel/ftrace.c | 80 ++---
arch/riscv/kernel/mcount-dyn.S | 9 +++--
3 files changed, 62 insertions(+), 31 deletions(-)
diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
index 5f81c53dbfd9..7199383f8c02 100644
--- a/arch/riscv/include/asm/ftrace.h
+++ b/arch/riscv/include/asm/ftrace.h
@@ -81,6 +81,7 @@ struct dyn_arch_ftrace {
#define JALR_T0(0x000282e7)
#define AUIPC_T0 (0x0297)
#define NOP4 (0x0013)
+#define JALR_RANGE (JALR_SIGN_MASK - 1)
#define to_jalr_t0(offset) \
(((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
@@ -118,6 +119,9 @@ do {
\
* Let auipc+jalr be the basic *mcount unit*, so we make it 8 bytes here.
*/
#define MCOUNT_INSN_SIZE 8
+#define MCOUNT_AUIPC_SIZE 4
+#define MCOUNT_JALR_SIZE 4
+#define MCOUNT_NOP4_SIZE 4
#ifndef __ASSEMBLY__
struct dyn_ftrace;
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 87cbd86576b2..f3b09f2d3ecc 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -64,42 +64,64 @@ static int ftrace_check_current_call(unsigned long hook_pos,
return 0;
}
-static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
- bool enable, bool ra)
+static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
bool validate)
{
unsigned int call[2];
- unsigned int nops[2] = {NOP4, NOP4};
+ unsigned int replaced[2];
+
+ make_call_t0(hook_pos, target, call);
- if (ra)
- make_call_ra(hook_pos, target, call);
- else
- make_call_t0(hook_pos, target, call);
+ if (validate) {
+ /*
+* Read the text we want to modify;
+* return must be -EFAULT on read error
+*/
+ if (copy_from_kernel_nofault(replaced, (void *)hook_pos,
+MCOUNT_INSN_SIZE))
+ return -EFAULT;
+
+ if (replaced[0] != call[0]) {
+ pr_err("%p: expected (%08x) but got (%08x)\n",
+ (void *)hook_pos, call[0], replaced[0]);
+ return -EINVAL;
+ }
+ }
- /* Replace the auipc-jalr pair at once. Return -EPERM on write error. */
- if (patch_insn_write((void *)hook_pos, enable ? call : nops,
MCOUNT_INSN_SIZE))
+ /* Replace the jalr at once. Return -EPERM on write error. */
+ if (patch_insn_write((void *)(hook_pos + MCOUNT_AUIPC_SIZE), call + 1,
MCOUNT_JALR_SIZE))
return -EPERM;
return 0;
}
-int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+static int __ftrace_modify_call_site(ftrace_func_t *hook_pos, ftrace_func_t
target, bool enable)
{
- unsigned int call[2];
+ ftrace_func_t call = target;
+ ftrace_func_t nops = _stub;
- make_call_t0(rec->ip, addr, call);
-
- if (patch_insn_write((void *)rec->ip, call, MCOUNT_INSN_SIZE))
- return -EPERM;
+ WRITE_ONCE(*hook_pos, enable ? call : nops);
return 0;
}
+int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+ unsigned long distance, orig_addr;
+
+ orig_addr = (unsigned long)_caller;
+ distance = addr > orig_addr ? addr - orig_addr : orig_addr - addr;
+ if (distance > JALR_RANGE)
+ return -EINVAL;
+
+ return __ftrace_modify_call(rec->ip, addr, false);
+}
+
int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
unsigned long addr)
{
- unsigned int nops[2] = {NOP4, NOP4};
+ unsigned int nops[1] = {NOP4};
- if (patch_insn_write((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
+ if (patch_insn_write((void
[PATCH 4/8] riscv: ftrace: align patchable functions to 4 Byte boundary
We are changing ftrace code patching in order to remove dependency from stop_machine() and enable kernel preemption. This requires us to align functions entry at a 4-B align address. However, -falign-functions on older versions of GCC alone was not strong enoungh to align all functions. In fact, cold functions are not aligned after turning on optimizations. We consider this is a bug in GCC and turn off guess-branch-probility as a workaround to align all functions. GCC bug id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345 The option -fmin-function-alignment is able to align all functions properly on newer versions of gcc. So, we add a cc-option to test if the toolchain supports it. Suggested-by: Evgenii Shatokhin Signed-off-by: Andy Chiu --- arch/riscv/Kconfig | 1 + arch/riscv/Makefile | 7 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index b94176e25be1..80b8d48e1e46 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -203,6 +203,7 @@ config CLANG_SUPPORTS_DYNAMIC_FTRACE config GCC_SUPPORTS_DYNAMIC_FTRACE def_bool CC_IS_GCC depends on $(cc-option,-fpatchable-function-entry=8) + depends on $(cc-option,-fmin-function-alignment=4) || !RISCV_ISA_C config HAVE_SHADOW_CALL_STACK def_bool $(cc-option,-fsanitize=shadow-call-stack) diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile index 06de9d365088..74628ad8dcf8 100644 --- a/arch/riscv/Makefile +++ b/arch/riscv/Makefile @@ -14,8 +14,13 @@ endif ifeq ($(CONFIG_DYNAMIC_FTRACE),y) LDFLAGS_vmlinux += --no-relax KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY +ifeq ($(CONFIG_CC_IS_CLANG),y) + cflags_ftrace_align := -falign-functions=4 +else + cflags_ftrace_align := -fmin-function-alignment=4 +endif ifeq ($(CONFIG_RISCV_ISA_C),y) - CC_FLAGS_FTRACE := -fpatchable-function-entry=4 + CC_FLAGS_FTRACE := -fpatchable-function-entry=4 $(cflags_ftrace_align) else CC_FLAGS_FTRACE := -fpatchable-function-entry=2 endif -- 2.43.0
[PATCH 3/8] riscv: ftrace: support fastcc in Clang for WITH_ARGS
Some caller-saved registers which are not defined as function arguments
in the ABI can still be passed as arguments when the kernel is compiled
with Clang. As a result, we must save and restore those registers to
prevent ftrace from clobbering them.
- [1]: https://reviews.llvm.org/D68559
Reported-by: Evgenii Shatokhin
Closes:
https://lore.kernel.org/linux-riscv/7e7c7914-445d-426d-89a0-59a9199c4...@yadro.com/
Signed-off-by: Andy Chiu
---
arch/riscv/include/asm/ftrace.h | 7 +++
arch/riscv/kernel/asm-offsets.c | 7 +++
arch/riscv/kernel/mcount-dyn.S | 16 ++--
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
index 9eb31a7ea0aa..5f81c53dbfd9 100644
--- a/arch/riscv/include/asm/ftrace.h
+++ b/arch/riscv/include/asm/ftrace.h
@@ -144,6 +144,13 @@ struct ftrace_regs {
unsigned long a5;
unsigned long a6;
unsigned long a7;
+#ifdef CONFIG_CC_IS_CLANG
+ unsigned long t2;
+ unsigned long t3;
+ unsigned long t4;
+ unsigned long t5;
+ unsigned long t6;
+#endif
};
};
};
diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index b09ca5f944f7..db5a26fcc9ae 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -497,6 +497,13 @@ void asm_offsets(void)
DEFINE(FREGS_SP,offsetof(struct ftrace_regs, sp));
DEFINE(FREGS_S0,offsetof(struct ftrace_regs, s0));
DEFINE(FREGS_T1,offsetof(struct ftrace_regs, t1));
+#ifdef CONFIG_CC_IS_CLANG
+ DEFINE(FREGS_T2,offsetof(struct ftrace_regs, t2));
+ DEFINE(FREGS_T3,offsetof(struct ftrace_regs, t3));
+ DEFINE(FREGS_T4,offsetof(struct ftrace_regs, t4));
+ DEFINE(FREGS_T5,offsetof(struct ftrace_regs, t5));
+ DEFINE(FREGS_T6,offsetof(struct ftrace_regs, t6));
+#endif
DEFINE(FREGS_A0,offsetof(struct ftrace_regs, a0));
DEFINE(FREGS_A1,offsetof(struct ftrace_regs, a1));
DEFINE(FREGS_A2,offsetof(struct ftrace_regs, a2));
diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
index 745dd4c4a69c..e988bd26b28b 100644
--- a/arch/riscv/kernel/mcount-dyn.S
+++ b/arch/riscv/kernel/mcount-dyn.S
@@ -96,7 +96,13 @@
REG_S x8, FREGS_S0(sp)
#endif
REG_S x6, FREGS_T1(sp)
-
+#ifdef CONFIG_CC_IS_CLANG
+ REG_S x7, FREGS_T2(sp)
+ REG_S x28, FREGS_T3(sp)
+ REG_S x29, FREGS_T4(sp)
+ REG_S x30, FREGS_T5(sp)
+ REG_S x31, FREGS_T6(sp)
+#endif
// save the arguments
REG_S x10, FREGS_A0(sp)
REG_S x11, FREGS_A1(sp)
@@ -115,7 +121,13 @@
REG_L x8, FREGS_S0(sp)
#endif
REG_L x6, FREGS_T1(sp)
-
+#ifdef CONFIG_CC_IS_CLANG
+ REG_L x7, FREGS_T2(sp)
+ REG_L x28, FREGS_T3(sp)
+ REG_L x29, FREGS_T4(sp)
+ REG_L x30, FREGS_T5(sp)
+ REG_L x31, FREGS_T6(sp)
+#endif
// restore the arguments
REG_L x10, FREGS_A0(sp)
REG_L x11, FREGS_A1(sp)
--
2.43.0
[PATCH 2/8] tracing: do not trace kernel_text_address()
kernel_text_address() and __kernel_text_address() are called in
arch_stack_walk() of riscv. This results in excess amount of un-related
traces when the kernel is compiled with CONFIG_TRACE_IRQFLAGS. The
situation worsens when function_graph is active, as it calls
local_irq_save/restore in each function's entry/exit. This patch adds
both functions to notrace, so they won't show up on the trace records.
Signed-off-by: Andy Chiu
---
kernel/extable.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/extable.c b/kernel/extable.c
index 71f482581cab..d03fa462fa8b 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -74,7 +74,7 @@ int notrace core_kernel_text(unsigned long addr)
return 0;
}
-int __kernel_text_address(unsigned long addr)
+int notrace __kernel_text_address(unsigned long addr)
{
if (kernel_text_address(addr))
return 1;
@@ -91,7 +91,7 @@ int __kernel_text_address(unsigned long addr)
return 0;
}
-int kernel_text_address(unsigned long addr)
+int notrace kernel_text_address(unsigned long addr)
{
bool no_rcu;
int ret = 1;
--
2.43.0
[PATCH 1/8] riscv: stacktrace: convert arch_stack_walk() to noinstr
arch_stack_walk() is called intensively in function_graph when the
kernel is compiled with CONFIG_TRACE_IRQFLAGS. As a result, the kernel
logs a lot of arch_stack_walk and its sub-functions into the ftrace
buffer. However, these functions should not appear on the trace log
because they are part of the ftrace itself. This patch references what
arm64 does for the smae function. So it further prevent the re-enter
kprobe issue, which is also possible on riscv.
Related-to: commit 0fbcd8abf337 ("arm64: Prohibit instrumentation on
arch_stack_walk()")
Fixes: 680341382da5 ("riscv: add CALLER_ADDRx support")
Signed-off-by: Andy Chiu
---
arch/riscv/kernel/stacktrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 528ec7cc9a62..0d3f00eb0bae 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -156,7 +156,7 @@ unsigned long __get_wchan(struct task_struct *task)
return pc;
}
-noinline void arch_stack_walk(stack_trace_consume_fn consume_entry, void
*cookie,
+noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
void *cookie,
struct task_struct *task, struct pt_regs *regs)
{
walk_stackframe(task, regs, consume_entry, cookie);
--
2.43.0
[PATCH 0/8] riscv: ftrace: atmoic patching and preempt improvements
This series makes atmoic code patching possible in riscv ftrace. A direct benefit of this is that we can get rid of stop_machine() when patching function entries. This also makes it possible to run ftrace with full kernel preemption. Before this series, the kernel initializes patchable function entries to NOP4 + NOP4. To start tracing, it updates entries to AUIPC + JALR while holding other cores in stop_machine. stop_machine() is required because it is impossible to update 2 instructions, and be seen atomically. And preemption must have to be prevented, as kernel preemption allows process to be scheduled out while executing on one of these instruction pairs. This series addresses the problem by initializing the first NOP4 to AUIPC. So, atmoic patching is possible because the kernel only has to update one instruction. As long as the instruction is naturally aligned, then it is expected to be updated atomically. However, the address range of the ftrace trampoline is limited to +-2K from ftrace_caller after appplying this series. This issue is expected to be solved by Puranjay's CALL_OPS, where it adds 8B naturally align data in front of pacthable functions and can use it to direct execution out to any custom trampolines. The series is composed by two parts. The first part (1-3) cleans up existing issues that was found during testing of and not caused by the implementation. The second part modifies the ftrace code patching mechanism (4-6) as mentioned above. Then prepare ftrace to be able to run with kernel preemption (7,8) --- Andy Chiu (8): riscv: stacktrace: convert arch_stack_walk() to noinstr tracing: do not trace kernel_text_address() riscv: ftrace: support fastcc in Clang for WITH_ARGS riscv: ftrace: align patchable functions to 4 Byte boundary riscv: ftrace: prepare ftrace for atomic code patching riscv: ftrace: do not use stop_machine to update code riscv: vector: Support calling schedule() for preemptible Vector riscv: ftrace: support PREEMPT arch/riscv/Kconfig | 3 +- arch/riscv/Makefile| 7 +- arch/riscv/include/asm/ftrace.h| 11 +++ arch/riscv/include/asm/processor.h | 5 ++ arch/riscv/include/asm/vector.h| 22 +- arch/riscv/kernel/asm-offsets.c| 7 ++ arch/riscv/kernel/ftrace.c | 133 - arch/riscv/kernel/mcount-dyn.S | 25 +-- arch/riscv/kernel/stacktrace.c | 2 +- kernel/extable.c | 4 +- 10 files changed, 129 insertions(+), 90 deletions(-) --- base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0 change-id: 20240613-dev-andyc-dyn-ftrace-v4-941d4a00ea19 Best regards, -- Andy Chiu
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman wrote: > On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > > > wrote: ... > > > > > ret = stk3310_init(indio_dev); > > > > > if (ret < 0) > > > > > - return ret; > > > > > + goto err_vdd_disable; > > > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > > IRQ. Note, that the original code likely has a bug which sets states > > > > before disabling IRQ and removing a handler. > > > > > > How so? stk3310_init is called before enabling the interrupt. > > > > Exactly, IRQ is registered with devm and hence the error path and > > remove stages will got it in a wrong order. > > Makes no sense. Huh?! > IRQ is not enabled here, yet. So in error path, the code will > just disable the regulator and devm will unref it later on. IRQ doesn't enter > the picture here at all in the error path. Error path _after_ IRQ handler has been _successfully_ installed. And complete ->remove() stage. > > > Original code has a bug that IRQ is enabled before registering the > > > IIO device, > > > > Indeed, but this is another bug. > > > > > so if IRQ is triggered before registration, iio_push_event > > > from IRQ handler may be called on a not yet registered IIO device. > > > > > > Never saw it happen, though. :) > > > > Because nobody cares enough to enable DEBUG_SHIRQ. > > Nice debug tool. I bet it makes quite a mess when enabled. :) FWIW, I have had it enabled for ages, but I have only a few devices, so I fixed a few cases in the past WRT shared IRQ issues. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > wrote: ... > > > ret = stk3310_init(indio_dev); > > > if (ret < 0) > > > - return ret; > > > + goto err_vdd_disable; > > > > This is wrong. You will have the regulator being disabled _before_ > > IRQ. Note, that the original code likely has a bug which sets states > > before disabling IRQ and removing a handler. > > How so? stk3310_init is called before enabling the interrupt. Exactly, IRQ is registered with devm and hence the error path and remove stages will got it in a wrong order. > Original code has a bug that IRQ is enabled before registering the > IIO device, Indeed, but this is another bug. > so if IRQ is triggered before registration, iio_push_event > from IRQ handler may be called on a not yet registered IIO device. > > Never saw it happen, though. :) Because nobody cares enough to enable DEBUG_SHIRQ. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote:
>
> From: Ondrej Jirman
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
...
> ret = stk3310_init(indio_dev);
> if (ret < 0)
> - return ret;
> + goto err_vdd_disable;
This is wrong. You will have the regulator being disabled _before_
IRQ. Note, that the original code likely has a bug which sets states
before disabling IRQ and removing a handler.
Side note, you may make the driver neater with help of
struct device *dev = >dev;
defined in this patch.
...
> static int stk3310_suspend(struct device *dev)
> {
> struct stk3310_data *data;
> data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
Side note: This may be updated (in a separate change) to use
dev_get_drvdata() directly.
Jonathan, do we have something like iio_priv_from_drvdata(struct
device *dev)? Seems many drivers may utilise it.
> }
...
> static int stk3310_resume(struct device *dev)
Ditto.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 3/6] iio: light: stk3310: Manage LED power supply
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote: > > The stk3310 and stk3310 chips have an input for power to the infrared > LED. Add support for managing it's state. its ... > if (IS_ERR(data->vdd_reg)) > return dev_err_probe(>dev, ret, "get regulator vdd > failed\n"); > > + data->led_reg = devm_regulator_get(>dev, "leda"); > + if (IS_ERR(data->led_reg)) > + return dev_err_probe(>dev, ret, "get regulator led > failed\n"); Can't you use a bulk regulator API instead? -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Thu, Apr 18, 2024 at 8:50 PM Aren wrote: > On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 18, 2024 at 6:06 PM Aren wrote: > > > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > > > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan > > > > wrote: ... > > > > I forgot to check the order of freeing resources, be sure you have no > > > > devm_*() releases happening before this call. > > > > > > If I understand what you're saying, this should be fine. The driver just > > > uses devm to clean up acquired resources after remove is called. Or am I > > > missing something and resources could be freed before calling > > > stk3310_remove? > > > > I'm not objecting to that. The point here is that the resources should > > be freed in the reversed order. devm-allocated resources are deferred > > to be freed after the explicit driver ->remove() callback. At the end > > it should not interleave with each other, i.o.w. it should be > > probe: devm followed by non-devm > > remove: non-devm only. > > I think what you're describing is already the case, with the exception > of parts of the probe function not changed in this patch mixing > acquiring resources through devm with configuring the device. Okay, then we are fine! > I hope I'm not being dense, thanks for the clarification -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Thu, Apr 18, 2024 at 6:06 PM Aren wrote: > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan > > wrote: ... > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > > > + if (data->vdd_reg) > > > + regulator_disable(data->vdd_reg); > > > > I forgot to check the order of freeing resources, be sure you have no > > devm_*() releases happening before this call. > > If I understand what you're saying, this should be fine. The driver just > uses devm to clean up acquired resources after remove is called. Or am I > missing something and resources could be freed before calling > stk3310_remove? I'm not objecting to that. The point here is that the resources should be freed in the reversed order. devm-allocated resources are deferred to be freed after the explicit driver ->remove() callback. At the end it should not interleave with each other, i.o.w. it should be probe: devm followed by non-devm remove: non-devm only. -- With Best Regards, Andy Shevchenko
Re: [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan wrote:
>
> If the chip isn't powered, this call is likely to return an error.
> Without a log here the driver will silently fail to probe. Common errors
> are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
> isn't powered).
> ret = regmap_read(data->regmap, STK3310_REG_ID, );
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(>dev, "failed to read chip id: %d", ret);
> return ret;
> + }
Briefly looking at the code it seems that this one is strictly part of
the probe phase, which means we may use
return dev_err_probe(...);
pattern. Yet, you may add another patch to clean up all of them:
_probe(), _init(), _regmap_init() to use the same pattern everywhere.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan wrote:
>
> From: Ondrej Jirman
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
...
> #include
> #include
> #include
> +#include
Move it to be ordered and add a blank line to separate iio/*.h group.
...
> + data->vdd_reg = devm_regulator_get_optional(>dev, "vdd");
> + if (IS_ERR(data->vdd_reg)) {
> + ret = PTR_ERR(data->vdd_reg);
> + if (ret == -ENODEV)
> + data->vdd_reg = NULL;
> + else
Redundant 'else' when you follow the pattern "check for error condition first".
> + return dev_err_probe(>dev, ret,
> +"get regulator vdd failed\n");
> + }
...
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret)
> + return dev_err_probe(>dev, ret,
> +"regulator vdd enable failed\n");
> +
> + usleep_range(1000, 2000);
fsleep()
> + }
...
> stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
I forgot to check the order of freeing resources, be sure you have no
devm_*() releases happening before this call.
...
> + usleep_range(1000, 2000);
fsleep()
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] [v3] module: don't ignore sysfs_create_link() failures
On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The sysfs_create_link() return code is marked as __must_check, but the
> module_add_driver() function tries hard to not care, by assigning the
> return code to a variable. When building with 'make W=1', gcc still
> warns because this variable is only assigned but not used:
>
> drivers/base/module.c: In function 'module_add_driver':
> drivers/base/module.c:36:6: warning: variable 'no_warn' set but not used
> [-Wunused-but-set-variable]
>
> Rework the code to properly unwind and return the error code to the
> caller. My reading of the original code was that it tries to
> not fail when the links already exist, so keep ignoring -EEXIST
> errors.
> Cc: Luis Chamberlain
> Cc: linux-modu...@vger.kernel.org
> Cc: Greg Kroah-Hartman
> Cc: "Rafael J. Wysocki"
Wondering if you can move these to be after --- to avoid polluting commit
message. This will have the same effect and be archived on lore. But on
pros side it will unload the commit message(s) from unneeded noise.
...
> + error = module_add_driver(drv->owner, drv);
> + if (error) {
> + printk(KERN_ERR "%s: failed to create module links for %s\n",
> + __func__, drv->name);
What's wrong with pr_err()? Even if it's not a style used, in a new pieces of
code this can be improved beforehand. So, we will reduce a technical debt, and
not adding to it.
> + goto out_detach;
> + }
...
> +int module_add_driver(struct module *mod, struct device_driver *drv)
> {
> char *driver_name;
> - int no_warn;
> + int ret;
I would move it...
> struct module_kobject *mk = NULL;
...to be here.
--
With Best Regards,
Andy Shevchenko
Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
On Thu, Mar 21, 2024 at 4:48 PM Björn Töpel wrote: > > Andy, > > Pulling out the A option: > > >> > A) Use auipc/jalr, only patch jalr to take us to a common > >> >dispatcher/trampoline > >> > > >> > | # probably on a data cache-line != func > >> > .text to avoid ping-pong > >> > | ... > >> > | func: > >> > | ...make sure ra isn't messed up... > >> > | aupic > >> > | nop <=> jalr # Text patch point -> common_dispatch > >> > | ACTUAL_FUNC > >> > | > >> > | common_dispatch: > >> > | load based on ra > >> > | jalr > >> > | ... > >> > > >> > The auipc is never touched, and will be overhead. Also, we need a mv to > >> > store ra in a scratch register as well -- like Arm. We'll have two insn > >> > per-caller overhead for a disabled caller. > > > > My patch series takes a similar "in-function dispatch" approach. A > > difference is that the is > > embedded within each function entry. I'd like to have it moved to a > > run-time allocated array to reduce total text size. > > This is what arm64 has as well. It's a 8B + 1-2 dirt cheap movish like > instructions (save ra, prepare jump with auipc). I think that's a > reasonable overhead. > > > Another difference is that my series changes the first instruction to > > "j ACTUAL_FUNC" for the "ftrace disable" case. As long as the > > architecture guarantees the atomicity of the first instruction, then > > we are safe. For example, we are safe if the first instruction could > > only be "mv tmp, ra" or "j ACTUAL_FUNC". And since the loaded address is > > always valid, we can fix "mv + jalr" down so we don't have to > > play with the exception handler trick. The guarantee from arch would > > require ziccif (in RVA22) though, but I think it is the same for us > > (unless with stop_machine). For ziccif, I would rather call that out > > during boot than blindly assume. > > I'm maybe biased, but I'd prefer the A) over your version with the > unconditional jump. A) has the overhead of two, I'd say, free > instructions (again "Meten is Weten!" ;-)). Yes, I'd also prefer A for less overall patch size. We can also optimize the overhead with a direct jump if that makes sense. Though, we need to sort out a way to map functions to corresponding trampolines. A direct way I could image is CALL_OPS'ish patching style, if the ftrace destination has to be patched in a per-function manner. For example: func_symbol: auipc t0, common_dispatch:high <=> j actual_func: jalr t0, common_dispatch:low(t0) common_dispatch: load t1, index + dispatch-list ld t1, 0(t1) jr t1 > > > However, one thing I am not very sure is: do we need a destination > > address in a "per-function" manner? It seems like most of the time the > > destination address can only be ftrace_call, or ftrace_regs_call. If > > the number of destination addresses is very few, then we could > > potentially reduce the size of > > . > > Yes, we do need a per-function manner. BPF, e.g., uses > dynamically/JIT:ed trampolines/targets. > > > > Björn Cheers, Andy
Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
mp, ra" or "j ACTUAL_FUNC". And since the loaded address is always valid, we can fix "mv + jalr" down so we don't have to play with the exception handler trick. The guarantee from arch would require ziccif (in RVA22) though, but I think it is the same for us (unless with stop_machine). For ziccif, I would rather call that out during boot than blindly assume. However, one thing I am not very sure is: do we need a destination address in a "per-function" manner? It seems like most of the time the destination address can only be ftrace_call, or ftrace_regs_call. If the number of destination addresses is very few, then we could potentially reduce the size of . > > There are 4 CMODX possiblities: >mv, nop: fully disabled, no problems >mv, jalr: We will jump to zero. We would need to have the inst > page/access fault handler take care of this case. Especially > if we align the instructions so that they can be patched > together, being interrupted in the middle and taking this > path will be rare. > ld, nop: no problems > ld, jalr: fully enabled, no problems > > Patching is a 64b store/sd, and we only need a fence.i at the end, since > we can handle all 4 possibilities. > > For the disabled case we'll have: > A) mv, aupic, nop > D) mv, aupic, mv, nop. > > Puranjay, I've flipped. Let's go Mark's CALL_OPS together with a new > text patch mechanism w/o stop_machine(). > > > Björn Cheers, Andy
Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
e space is a pointer to the ftrace entry. During boot, each function entry code is updated to perform a load and then take the jump from the 8-B space. When ftrace is disabled, we patch the first 4B-aligned instruction to a jump so as to skip the ftrace entry. We are still discussing with Alex to see if we have a better way to do it. If not then I'd update the patchset and re-send it. There's a pending improvement in the series to reduce complexity. The 8-B aligned space can be added before the function entry (just like your patch). > > > * Larger text size (function alignment + extra nops) BAD > > * Same direct call performance NEUTRAL > > * Same complicated text patching required NEUTRAL > > > > It would be interesting to see how the per-call performance would > > improve on x86 with CALL_OPS! ;-) > > If I remember from Steven's talk, x86 uses dynamically allocated trampolines > for per callsite tracers, would CALL_OPS provide better performance than that? > > > > > I'm trying to wrap my head if it makes sense to have it on RISC-V, given > > that we're a bit different from Arm64. Does the scale tip to the GOOD > > side? > > > > Oh, and we really need to see performance numbers on real HW! I have a > > VF2 that I could try this series on. > > It would be great if you can do it :D. > > Thanks, > Puranjay > > ___ > linux-riscv mailing list > linux-ri...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv - [1] https://yhbt.net/lore/all/cajf2gtsn3_cdysf9d8dt-br2wf_m8y02a09xgrq8kxi91sn...@mail.gmail.com/T/ Regards, Andy
Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
52 | - | > > > |0 | 1 |16136700 |161 | - | > > > |0 | 2 |15329500 |153 | - | > > > |0 | 10 |15148800 |151 | - | > > > |0 |100 |15746900 |157 | - | > > > |0 |200 |15737400 |157 | - | > > > |--++-++---| > > > |1 | 0 |47909000 |479 | 327 | > > > |1 | 1 |48297400 |482 | 330 | > > > |1 | 2 |47314100 |473 | 321 | > > > |1 | 10 |47844900 |478 | 326 | > > > |1 |100 |46591900 |465 | 313 | > > > |1 |200 |47178900 |471 | 319 | > > > |--++-++---| > > > |1 | 0 |46715800 |467 | 315 | > > > |2 | 0 | 155134500 | 1551 | 1399 | > > > | 10 | 0 | 442672800 | 4426 | 4274 | > > > | 100 | 0 | 4092353900 | 40923 | 40771 | > > > | 200 | 0 | 7135796400 | 71357 | 71205 | > > > +--++-++---+ > > > > > > Note: per-call overhead is estimated relative to the baseline case with > > > 0 relevant tracers and 0 irrelevant tracers. > > > > > > As can be seen from the above: > > > > > > a) Whenever there is a single relevant tracer function associated with a > > > tracee, the overhead of invoking the tracer is constant, and does not > > > scale with the number of tracers which are *not* associated with that > > > tracee. > > > > > > b) The overhead for a single relevant tracer has dropped to ~1/3 of the > > > overhead prior to this series (from 1035ns to 315ns). This is largely > > > due to permitting calls to dynamically-allocated ftrace_ops without > > > going through ftrace_ops_list_func. > > > > > > Why is this patch a RFC patch: > > > 1. I saw some rcu stalls on Qemu and need to debug them and see if they > > > were introduced by this patch. > > > > > > FYI, I'm currently working on debugging such issues (and other) with the > > *current* ftrace implementation, so probably not caused by your > > patchset. But keep debugging too, maybe this introduces other issues or > > even better, you'll find the root cause :) > > > > > > > 2. This needs to be tested thoroughly on real hardware. > > > 3. Seeking reviews to fix any fundamental problems with this patch that > > > I > > > might have missed due to my lack of RISC-V architecture knowledge. > > > 4. I would like to benchmark this on real hardware and put the results > > > in > > > the commit message. > > > > > > Signed-off-by: Puranjay Mohan > > > --- > > > arch/riscv/Kconfig | 2 ++ > > > arch/riscv/Makefile | 8 + > > > arch/riscv/include/asm/ftrace.h | 3 ++ > > > arch/riscv/kernel/asm-offsets.c | 3 ++ > > > arch/riscv/kernel/ftrace.c | 59 + > > > arch/riscv/kernel/mcount-dyn.S | 42 --- > > > 6 files changed, 112 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > > index 0bfcfec67ed5..e474742e23b2 100644 > > > --- a/arch/riscv/Kconfig > > > +++ b/arch/riscv/Kconfig > > > @@ -78,6 +78,7 @@ config RISCV > > > select EDAC_SUPPORT > > > select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && > > > !DYNAMIC_FTRACE) > > > select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE > > > + select FUNCTION_ALIGNMENT_8B if DYNAMIC_FTRACE_WITH_CALL_OPS > > > > > > A recent discussion [1] states that -falign-functions cannot guarantee > > this alignment for all code and that gcc developers came up with a new > > option [2]: WDYT? I have added Andy and Evgenii in +cc to help on that. > > I saw arm64 uses the same and assumes this guarantee, maybe it
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Mon, Nov 20, 2023 at 07:19:44PM +0200, Andy Shevchenko wrote: > On Mon, Nov 20, 2023 at 04:11:54PM +0100, Rafael J. Wysocki wrote: > > On Mon, Nov 20, 2023 at 4:03 PM Andy Shevchenko > > wrote: > > > On Thu, Oct 19, 2023 at 06:03:28PM -0700, Dan Williams wrote: > > > > Andy Shevchenko wrote: > > > > > The acpi_evaluate_dsm_typed() provides a way to check the type of the > > > > > object evaluated by _DSM call. Use it instead of open coded variant. > > > > > > > > Looks good to me. > > > > > > > > Reviewed-by: Dan Williams > > > > > > Thank you! > > > > > > Who is taking care of this? Rafael? > > > > I can apply it. > > Would be nice, thank you! Any news on this? -- With Best Regards, Andy Shevchenko
[v1] trace/hwlat: stop worker if !is_percpu_thread due to hotplug event
If the task happens to run after cpu hot-plug offline, then it would not
be running in a percpu_thread. Instead, it would be re-queued into a
UNBOUND workqueue. This would trigger a warning if we enable kernel
preemption.
Signed-off-by: Andy Chiu
---
kernel/trace/trace_hwlat.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_hwlat.c b/kernel/trace/trace_hwlat.c
index b791524a6536..87258ddc2141 100644
--- a/kernel/trace/trace_hwlat.c
+++ b/kernel/trace/trace_hwlat.c
@@ -511,7 +511,16 @@ static int start_cpu_kthread(unsigned int cpu)
static void hwlat_hotplug_workfn(struct work_struct *dummy)
{
struct trace_array *tr = hwlat_trace;
- unsigned int cpu = smp_processor_id();
+ unsigned int cpu;
+
+ /*
+* If the work is scheduled after CPU hotplug offline being invoked,
+* then it would be queued into UNBOUNDED workqueue
+*/
+ if (!is_percpu_thread())
+ return;
+
+ cpu = smp_processor_id();
mutex_lock(_types_lock);
mutex_lock(_data.lock);
--
2.43.0
[v1] trace/osnoise: prevent osnoise hotplog worker running in UNBOUND workqueue
smp_processor_id() should be called with migration disabled. This mean
we may safely call smp_processor_id() in percpu thread. However, this is
not the case if the work is (re-)queued into unbound workqueue, during
cpu-hotplog. So, detect and return early if this work happens to run on
an unbound wq.
Signed-off-by: Andy Chiu
---
kernel/trace/trace_osnoise.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index bd0d01d00fb9..cf7f716d3f35 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2068,7 +2068,12 @@ static int start_per_cpu_kthreads(void)
#ifdef CONFIG_HOTPLUG_CPU
static void osnoise_hotplug_workfn(struct work_struct *dummy)
{
- unsigned int cpu = smp_processor_id();
+ unsigned int cpu;
+
+ if (!is_percpu_thread())
+ return;
+
+ cpu = smp_processor_id();
mutex_lock(_types_lock);
--
2.43.0
Re: [PATCH] tracing histograms: Simplify parse_actions() function
On Mon, Jan 8, 2024 at 3:31 AM Steven Rostedt wrote:
>
> From: "Steven Rostedt (Google)"
>
> The parse_actions() function uses 'len = str_has_prefix()' to test which
> action is in the string being parsed. But then it goes and repeats the
> logic for each different action. This logic can be simplified and
> duplicate code can be removed as 'len' contains the length of the found
> prefix which should be used for all actions.
> Link: https://lore.kernel.org/all/20240107112044.6702c...@gandalf.local.home/
>
> Signed-off-by: Steven Rostedt (Google)
If you want Link to be formally a tag, you should drop the following
blank line.
> + if ((len = str_has_prefix(str, "onmatch(")))
> + hid = HANDLER_ONMATCH;
> + else if ((len = str_has_prefix(str, "onmax(")))
> + hid = HANDLER_ONMAX;
> + else if ((len = str_has_prefix(str, "onchange(")))
> + hid = HANDLER_ONCHANGE;
The repeating check for ( might be moved out as well after this like
if (str[len] != '(') {
// not sure if you need data to be assigned here as well
ret = -EINVAL;
...
}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 0/5] params: harden string ops and allocatio ops
On Fri, Dec 01, 2023 at 09:43:34AM -0800, Kees Cook wrote: > On Mon, 20 Nov 2023 17:11:41 +0200, Andy Shevchenko wrote: > > A couple of patches are for get the string ops, used in the module, > > slightly harden. On top a few cleanups. > > > > Since the main part is rather hardening, I think the Kees' tree is > > the best fit for the series. It also possible to route via Greg's > > sysfs (driver core?), but I'm open for another option(s). [...] > Applied to for-next/hardening, thanks! Awesome, thanks! -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 0/5] params: harden string ops and allocatio ops
On Mon, Nov 20, 2023 at 05:11:41PM +0200, Andy Shevchenko wrote: > A couple of patches are for get the string ops, used in the module, > slightly harden. On top a few cleanups. > > Since the main part is rather hardening, I think the Kees' tree is > the best fit for the series. It also possible to route via Greg's > sysfs (driver core?), but I'm open for another option(s). Kees, Greg, can you apply this series? Or should I do something about it? -- With Best Regards, Andy Shevchenko
[PATCH v3 2/5] params: Do not go over the limit when getting the string length
We can use strnlen() even on early stages and it prevents from
going over the string boundaries in case it's already too long.
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index 626fa8265932..f8e3c4139854 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -260,7 +260,10 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax);
int param_set_charp(const char *val, const struct kernel_param *kp)
{
- if (strlen(val) > 1024) {
+ size_t len, maxlen = 1024;
+
+ len = strnlen(val, maxlen + 1);
+ if (len == maxlen + 1) {
pr_err("%s: string parameter too long\n", kp->name);
return -ENOSPC;
}
@@ -270,7 +273,7 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
/* This is a hack. We can't kmalloc in early boot, and we
* don't need to; this mangled commandline is preserved. */
if (slab_is_available()) {
- *(char **)kp->arg = kmalloc_parameter(strlen(val)+1);
+ *(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
return -ENOMEM;
strcpy(*(char **)kp->arg, val);
@@ -508,7 +511,7 @@ int param_set_copystring(const char *val, const struct
kernel_param *kp)
{
const struct kparam_string *kps = kp->str;
- if (strlen(val)+1 > kps->maxlen) {
+ if (strnlen(val, kps->maxlen) == kps->maxlen) {
pr_err("%s: string doesn't fit in %u chars.\n",
kp->name, kps->maxlen-1);
return -ENOSPC;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 5/5] params: Fix multi-line comment style
The multi-line comment style in the file is rather arbitrary.
Make it follow the standard one.
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index eb55b32399b4..2e447f8ae183 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-or-later
-/* Helpers for initial module or kernel cmdline parsing
- Copyright (C) 2001 Rusty Russell.
-
-*/
+/*
+ * Helpers for initial module or kernel cmdline parsing
+ * Copyright (C) 2001 Rusty Russell.
+ */
#include
#include
#include
@@ -271,8 +271,10 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
maybe_kfree_parameter(*(char **)kp->arg);
- /* This is a hack. We can't kmalloc in early boot, and we
-* don't need to; this mangled commandline is preserved. */
+ /*
+* This is a hack. We can't kmalloc() in early boot, and we
+* don't need to; this mangled commandline is preserved.
+*/
if (slab_is_available()) {
*(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
@@ -743,8 +745,10 @@ void module_param_sysfs_remove(struct module *mod)
{
if (mod->mkobj.mp) {
sysfs_remove_group(>mkobj.kobj, >mkobj.mp->grp);
- /* We are positive that no one is using any param
-* attrs at this point. Deallocate immediately. */
+ /*
+* We are positive that no one is using any param
+* attrs at this point. Deallocate immediately.
+*/
free_module_param_attrs(>mkobj);
}
}
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 4/5] params: Sort headers
Sort the headers in alphabetic order in order to ease the maintenance for this part. Reviewed-by: Luis Chamberlain Reviewed-by: Kees Cook Signed-off-by: Andy Shevchenko --- kernel/params.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index c3a029fe183d..eb55b32399b4 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -3,18 +3,18 @@ Copyright (C) 2001 Rusty Russell. */ -#include -#include -#include -#include -#include -#include +#include #include #include +#include +#include +#include +#include +#include #include -#include -#include #include +#include +#include #ifdef CONFIG_SYSFS /* Protects all built-in parameters, modules use their own param_lock */ -- 2.43.0.rc1.1.gbec44491f096
[PATCH v3 1/5] params: Introduce the param_unknown_fn type
Introduce a new type for the callback to parse an unknown argument.
This unifies function prototypes which takes that as a parameter.
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
include/linux/moduleparam.h | 6 +++---
kernel/params.c | 8 ++--
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 4fa9726bc328..bfb85fd13e1f 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -385,6 +385,8 @@ extern bool parameq(const char *name1, const char *name2);
*/
extern bool parameqn(const char *name1, const char *name2, size_t n);
+typedef int (*parse_unknown_fn)(char *param, char *val, const char *doing,
void *arg);
+
/* Called on module insert or kernel boot */
extern char *parse_args(const char *name,
char *args,
@@ -392,9 +394,7 @@ extern char *parse_args(const char *name,
unsigned num,
s16 level_min,
s16 level_max,
- void *arg,
- int (*unknown)(char *param, char *val,
-const char *doing, void *arg));
+ void *arg, parse_unknown_fn unknown);
/* Called by module remove. */
#ifdef CONFIG_SYSFS
diff --git a/kernel/params.c b/kernel/params.c
index 2d4a0564697e..626fa8265932 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -120,9 +120,7 @@ static int parse_one(char *param,
unsigned num_params,
s16 min_level,
s16 max_level,
-void *arg,
-int (*handle_unknown)(char *param, char *val,
-const char *doing, void *arg))
+void *arg, parse_unknown_fn handle_unknown)
{
unsigned int i;
int err;
@@ -165,9 +163,7 @@ char *parse_args(const char *doing,
unsigned num,
s16 min_level,
s16 max_level,
-void *arg,
-int (*unknown)(char *param, char *val,
- const char *doing, void *arg))
+void *arg, parse_unknown_fn unknown)
{
char *param, *val, *err = NULL;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 3/5] params: Use size_add() for kmalloc()
Prevent allocations from integer overflow by using size_add().
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/params.c b/kernel/params.c
index f8e3c4139854..c3a029fe183d 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,7 +49,7 @@ static void *kmalloc_parameter(unsigned int size)
{
struct kmalloced_param *p;
- p = kmalloc(sizeof(*p) + size, GFP_KERNEL);
+ p = kmalloc(size_add(sizeof(*p), size), GFP_KERNEL);
if (!p)
return NULL;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 0/5] params: harden string ops and allocatio ops
A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups. Since the main part is rather hardening, I think the Kees' tree is the best fit for the series. It also possible to route via Greg's sysfs (driver core?), but I'm open for another option(s). Changelog v3: - added tags (Kees, Luis) Changelog v2: - dropped the s*printf() --> sysfs_emit() conversion as it revealed an issue, i.e. reuse getters with non-page-aligned pointer, which would be addressed separately - added cover letter and clarified the possible route for the series (Luis) Andy Shevchenko (5): params: Introduce the param_unknown_fn type params: Do not go over the limit when getting the string length params: Use size_add() for kmalloc() params: Sort headers params: Fix multi-line comment style include/linux/moduleparam.h | 6 ++-- kernel/params.c | 56 - 2 files changed, 33 insertions(+), 29 deletions(-) -- 2.43.0.rc1.1.gbec44491f096
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Thu, Oct 19, 2023 at 06:03:28PM -0700, Dan Williams wrote: > Andy Shevchenko wrote: > > The acpi_evaluate_dsm_typed() provides a way to check the type of the > > object evaluated by _DSM call. Use it instead of open coded variant. > > Looks good to me. > > Reviewed-by: Dan Williams Thank you! Who is taking care of this? Rafael? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Mon, Oct 02, 2023 at 04:54:58PM +0300, Andy Shevchenko wrote: > The acpi_evaluate_dsm_typed() provides a way to check the type of the > object evaluated by _DSM call. Use it instead of open coded variant. Dan, do you have any comments? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()
On Sat, Oct 14, 2023 at 12:20 AM Dan Williams wrote: > Wilczynski, Michal wrote: ... > "The new cleanup.h facilities that arrived in v6.5-rc1 can replace the > the usage of devm semantics in acpi_nfit_init_interleave_set(). That > routine appears to only be using devm to avoid goto statements. The new > __free() annotation at variable declaration time can achieve the same > effect more efficiently. > > There is no end user visible side effects of this patch, I was motivated > to send this cleanup to practice using the new helpers." The end-user side effect (educational and not run-time) is that: "One should really be careful about the scope of the devm_*() APIs and use of them just for the sake of the RAII replacement is not the best idea, while code is still working. Hence it gives a better example for whoever tries to use this code for educational purposes." -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
> wrote:
...
> > struct acpi_ac {
> > struct power_supply *charger;
> > struct power_supply_desc charger_desc;
> > - struct acpi_device *device;
> > + struct device *dev;
>
> I'm not convinced about this change.
>
> If I'm not mistaken, you only use the dev pointer above to get the
> ACPI_COMPANION() of it, but the latter is already found in _probe(),
> so it can be stored in struct acpi_ac for later use and then the dev
> pointer in there will not be necessary any more.
>
> That will save you a bunch of ACPI_HANDLE() evaluations and there's
> nothing wrong with using ac->device->handle. The patch will then
> become almost trivial AFAICS and if you really need to get from ac to
> the underlying platform device, a pointer to it can be added to struct
> acpi_ac without removing the ACPI device pointer from it.
The idea behind is to eliminate data duplication.
> > unsigned long long state;
> > struct notifier_block battery_nb;
> > };
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Fri, Oct 06, 2023 at 08:30:52PM +0300, Michal Wilczynski wrote:
> AC driver uses struct acpi_driver incorrectly to register itself. This
> is wrong as the instances of the ACPI devices are not meant to
> be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> Drop unnecessary casts from acpi_bus_generate_netlink_event() and
> acpi_notifier_call_chain().
>
> Add a blank line to distinguish pdev API vs local ACPI notify function.
...
> struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> - struct acpi_device *device;
> + struct device *dev;
> unsigned long long state;
> struct notifier_block battery_nb;
> };
When changing this, also makes sense just to check if the moving a member in
the data structure makes code shorter, but it's not a show stopper.
...
> - status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
> + status = acpi_evaluate_integer(ACPI_HANDLE(ac->dev), "_PSR", NULL,
> >state);
> if (ACPI_FAILURE(status)) {
> - acpi_handle_info(ac->device->handle,
> + acpi_handle_info(ACPI_HANDLE(ac->dev),
Can we call ACPI_HANDLE() only once and cache that in a local variable and use
in all places?
...
> - struct acpi_ac *ac = acpi_driver_data(device);
> + struct acpi_ac *ac = data;
> + struct acpi_device *device = ACPI_COMPANION(ac->dev);
>
> switch (event) {
> default:
> - acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
> + acpi_handle_debug(ACPI_HANDLE(ac->dev), "Unsupported event
> [0x%x]\n",
> event);
Does it makes any sense now? Basically it duplicates the ACPI_COMPANION() call
as Rafael pointed out in previous version discussion.
> fallthrough;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Mon, Oct 02, 2023 at 10:27:02PM +0200, Wilczynski, Michal wrote:
> On 10/2/2023 3:54 PM, Andy Shevchenko wrote:
...
> > + out_obj = acpi_evaluate_dsm_typed(handle, guid, revid, func, _obj,
> > ACPI_TYPE_BUFFER);
>
> This line is 90 characters long, wouldn't it be better to split it ?
I dunno it's a problem, but if people insist, I can redo that.
...
> > + if (!out_obj || out_obj->buffer.length < sizeof(smart)) {
> > dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
> > dev_name(dev));
>
> While at it maybe fix alignment ? :-)
I don't think it's in scope of this change.
> > ACPI_FREE(out_obj);
>
> Just nitpicks, functionally code seems correct to me.
> Reviewed-by: Michal Wilczynski
Thank you!
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
The acpi_evaluate_dsm_typed() provides a way to check the type of the
object evaluated by _DSM call. Use it instead of open coded variant.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/nfit/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index f96bf32cd368..280da408c02c 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1737,9 +1737,8 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem
*nfit_mem)
if ((nfit_mem->dsm_mask & (1 << func)) == 0)
return;
- out_obj = acpi_evaluate_dsm(handle, guid, revid, func, _obj);
- if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER
- || out_obj->buffer.length < sizeof(smart)) {
+ out_obj = acpi_evaluate_dsm_typed(handle, guid, revid, func, _obj,
ACPI_TYPE_BUFFER);
+ if (!out_obj || out_obj->buffer.length < sizeof(smart)) {
dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
dev_name(dev));
ACPI_FREE(out_obj);
--
2.40.0.1.gaa8946217a0b
[PATCH v2 4/5] params: Sort headers
Sort the headers in alphabetic order in order to ease the maintenance for this part. Signed-off-by: Andy Shevchenko --- kernel/params.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index c3a029fe183d..eb55b32399b4 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -3,18 +3,18 @@ Copyright (C) 2001 Rusty Russell. */ +#include +#include +#include +#include #include #include -#include -#include #include #include -#include -#include #include -#include -#include #include +#include +#include #ifdef CONFIG_SYSFS /* Protects all built-in parameters, modules use their own param_lock */ -- 2.40.0.1.gaa8946217a0b
[PATCH v2 1/5] params: Introduce the param_unknown_fn type
Introduce a new type for the callback to parse an unknown argument.
This unifies function prototypes which takes that as a parameter.
Signed-off-by: Andy Shevchenko
---
include/linux/moduleparam.h | 6 +++---
kernel/params.c | 8 ++--
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 4fa9726bc328..bfb85fd13e1f 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -385,6 +385,8 @@ extern bool parameq(const char *name1, const char *name2);
*/
extern bool parameqn(const char *name1, const char *name2, size_t n);
+typedef int (*parse_unknown_fn)(char *param, char *val, const char *doing,
void *arg);
+
/* Called on module insert or kernel boot */
extern char *parse_args(const char *name,
char *args,
@@ -392,9 +394,7 @@ extern char *parse_args(const char *name,
unsigned num,
s16 level_min,
s16 level_max,
- void *arg,
- int (*unknown)(char *param, char *val,
-const char *doing, void *arg));
+ void *arg, parse_unknown_fn unknown);
/* Called by module remove. */
#ifdef CONFIG_SYSFS
diff --git a/kernel/params.c b/kernel/params.c
index 2d4a0564697e..626fa8265932 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -120,9 +120,7 @@ static int parse_one(char *param,
unsigned num_params,
s16 min_level,
s16 max_level,
-void *arg,
-int (*handle_unknown)(char *param, char *val,
-const char *doing, void *arg))
+void *arg, parse_unknown_fn handle_unknown)
{
unsigned int i;
int err;
@@ -165,9 +163,7 @@ char *parse_args(const char *doing,
unsigned num,
s16 min_level,
s16 max_level,
-void *arg,
-int (*unknown)(char *param, char *val,
- const char *doing, void *arg))
+void *arg, parse_unknown_fn unknown)
{
char *param, *val, *err = NULL;
--
2.40.0.1.gaa8946217a0b
[PATCH v2 3/5] params: Use size_add() for kmalloc()
Prevent allocations from integer overflow by using size_add().
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/params.c b/kernel/params.c
index f8e3c4139854..c3a029fe183d 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,7 +49,7 @@ static void *kmalloc_parameter(unsigned int size)
{
struct kmalloced_param *p;
- p = kmalloc(sizeof(*p) + size, GFP_KERNEL);
+ p = kmalloc(size_add(sizeof(*p), size), GFP_KERNEL);
if (!p)
return NULL;
--
2.40.0.1.gaa8946217a0b
[PATCH v2 0/5] params: harden string ops and allocatio ops
A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups. Since the main part is rather hardening, I think the Kees' tree is the best fit for the series, but I'm open for another option(s). Changelog v2: - dropped the s*printf() --> sysfs_emit() conversion as it revealed an issue, i.e. reuse getters with non-page-aligned pointer, which would be addressed separately - added cover letter and clarified the possible route for the series (Luis) Andy Shevchenko (5): params: Introduce the param_unknown_fn type params: Do not go over the limit when getting the string length params: Use size_add() for kmalloc() params: Sort headers params: Fix multi-line comment style include/linux/moduleparam.h | 6 ++--- kernel/params.c | 52 - 2 files changed, 31 insertions(+), 27 deletions(-) -- 2.40.0.1.gaa8946217a0b
[PATCH v2 2/5] params: Do not go over the limit when getting the string length
We can use strnlen() even on early stages and it prevents from
going over the string boundaries in case it's already too long.
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index 626fa8265932..f8e3c4139854 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -260,7 +260,10 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax);
int param_set_charp(const char *val, const struct kernel_param *kp)
{
- if (strlen(val) > 1024) {
+ size_t len, maxlen = 1024;
+
+ len = strnlen(val, maxlen + 1);
+ if (len == maxlen + 1) {
pr_err("%s: string parameter too long\n", kp->name);
return -ENOSPC;
}
@@ -270,7 +273,7 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
/* This is a hack. We can't kmalloc in early boot, and we
* don't need to; this mangled commandline is preserved. */
if (slab_is_available()) {
- *(char **)kp->arg = kmalloc_parameter(strlen(val)+1);
+ *(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
return -ENOMEM;
strcpy(*(char **)kp->arg, val);
@@ -508,7 +511,7 @@ int param_set_copystring(const char *val, const struct
kernel_param *kp)
{
const struct kparam_string *kps = kp->str;
- if (strlen(val)+1 > kps->maxlen) {
+ if (strnlen(val, kps->maxlen) == kps->maxlen) {
pr_err("%s: string doesn't fit in %u chars.\n",
kp->name, kps->maxlen-1);
return -ENOSPC;
--
2.40.0.1.gaa8946217a0b
[PATCH v2 5/5] params: Fix multi-line comment style
The multi-line comment style in the file is rather arbitrary.
Make it follow the standard one.
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index eb55b32399b4..2e447f8ae183 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-or-later
-/* Helpers for initial module or kernel cmdline parsing
- Copyright (C) 2001 Rusty Russell.
-
-*/
+/*
+ * Helpers for initial module or kernel cmdline parsing
+ * Copyright (C) 2001 Rusty Russell.
+ */
#include
#include
#include
@@ -271,8 +271,10 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
maybe_kfree_parameter(*(char **)kp->arg);
- /* This is a hack. We can't kmalloc in early boot, and we
-* don't need to; this mangled commandline is preserved. */
+ /*
+* This is a hack. We can't kmalloc() in early boot, and we
+* don't need to; this mangled commandline is preserved.
+*/
if (slab_is_available()) {
*(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
@@ -743,8 +745,10 @@ void module_param_sysfs_remove(struct module *mod)
{
if (mod->mkobj.mp) {
sysfs_remove_group(>mkobj.kobj, >mkobj.mp->grp);
- /* We are positive that no one is using any param
-* attrs at this point. Deallocate immediately. */
+ /*
+* We are positive that no one is using any param
+* attrs at this point. Deallocate immediately.
+*/
free_module_param_attrs(>mkobj);
}
}
--
2.40.0.1.gaa8946217a0b
Re: [PATCH v1 2/2] ACPI: NFIT: Use modern scope based rollback
On Tue, Sep 26, 2023 at 09:45:20PM +0300, Michal Wilczynski wrote: > Change rollback in acpi_nfit_init_interleave_set() to use modern scope > based attribute __free(). This is similar to C++ RAII and is a preferred > way for handling local memory allocations. LGTM, Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/2] ACPI: NFIT: Fix memory leak, and local use of devm_*()
On Tue, Sep 26, 2023 at 09:45:19PM +0300, Michal Wilczynski wrote: > devm_*() family of functions purpose is managing memory attached to a > device. So in general it should only be used for allocations that should > last for the whole lifecycle of the device. This is not the case for > acpi_nfit_init_interleave_set(). There are two allocations that are only > used locally in this function. What's more - if the function exits on > error path memory is never freed. It's still attached to dev and would > be freed on device detach, so this leak could be called a 'local leak'. > > Fix this by switching from devm_kcalloc() to kcalloc(), and adding > proper rollback. LGTM, Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 9/9] ACPI: NFIT: Don't use KBUILD_MODNAME for driver name
On Mon, Sep 25, 2023 at 05:48:42PM +0300, Michal Wilczynski wrote: > Driver name is part of the ABI, so it should be hard-coded, as ABI > should be always kept backward compatible. Prevent ABI from changing > accidentally in case KBUILD_MODNAME change. This is up to maintainers, probably we won't have any users outside of existing model (instantiating via ACPI ID). All the above is "strictly speaking"... -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] lib/string_helpers: Don't copy a tail in kstrdup_and_replace() if 'new' is \0
On Wed, Sep 13, 2023 at 12:45:57PM +0300, Andy Shevchenko wrote: > The kstrdup_and_replace() takes two characters, old and new, to replace > former with latter after the copying of the original string. But in case > when new is a NUL, there is no point to copy the rest of the string, > the contract with the callers is that that the function returns a > NUL-terminated string and not a buffer of the size filled with a given > data. With this we can optimize the memory consumption by copying only > meaningful part of the original string and drop the rest. Thinking about this more, I self NAK this. If the caller knows the size of the original message it can be handy to make a copy and replace all occurrences of old by NUL. This will be an optimized implementation of strsep(str, "$OLD"). -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] lib/string_helpers: Don't copy a tail in kstrdup_and_replace() if 'new' is \0
The kstrdup_and_replace() takes two characters, old and new, to replace
former with latter after the copying of the original string. But in case
when new is a NUL, there is no point to copy the rest of the string,
the contract with the callers is that that the function returns a
NUL-terminated string and not a buffer of the size filled with a given
data. With this we can optimize the memory consumption by copying only
meaningful part of the original string and drop the rest.
Signed-off-by: Andy Shevchenko
---
The first user of this is pending:
https://lore.kernel.org/platform-driver-x86/20230913092701.440959-1-andriy.shevche...@linux.intel.com/
lib/string_helpers.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 7713f73e66b0..e385bf3cc2de 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -723,11 +723,17 @@ EXPORT_SYMBOL_GPL(kstrdup_quotable_file);
/*
* Returns duplicate string in which the @old characters are replaced by @new.
+ *
+ * If @new is NUL, copy the string up to the first occurrence of @old, which
+ * will be replaced by a NUL.
*/
char *kstrdup_and_replace(const char *src, char old, char new, gfp_t gfp)
{
char *dst;
+ if (new == '\0')
+ return kmemdup_nul(src, strchrnul(src, old) - src, gfp);
+
dst = kstrdup(src, gfp);
if (!dst)
return NULL;
--
2.40.0.1.gaa8946217a0b
Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
>
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.
> This issue was detected by using the Coccinelle software.
>
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid
> helpers")
Same issues as per patch 1.
...
> - if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> + if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(>dev), 16) != 0)
If parent_uuid is of uuid_t type, you better to replace memcmp() with
uuid_equal().
> return -ENODEV;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] nvdimm/namespace: return uuid_null only once in nd_dev_to_uuid()
On Thu, Jul 14, 2022 at 11:24:05AM -0700, Dan Williams wrote: > Andy Shevchenko wrote: > > Refactor nd_dev_to_uuid() in order to make code shorter and cleaner > > by joining conditions and hence returning uuid_null only once. > > Apologies for the delay, applied for v5.20. No problem and thanks! P.S. One patch out of three is a fix, would be nice to have it in v5.19 release. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] nvdimm/namespace: drop nested variable in create_namespace_pmem()
On Tue, Jun 07, 2022 at 07:49:37PM +0300, Andy Shevchenko wrote: > Kernel build bot reported: > > namespace_devs.c:1991:10: warning: Local variable 'uuid' shadows outer > variable [shadowVariable] > > Refactor create_namespace_pmem() by dropping a nested version of > the same variable. Any comments on this and other two patches? -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] nvdimm/namespace: drop nested variable in create_namespace_pmem()
Kernel build bot reported:
namespace_devs.c:1991:10: warning: Local variable 'uuid' shadows outer
variable [shadowVariable]
Refactor create_namespace_pmem() by dropping a nested version of
the same variable.
Fixes: d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")
Reported-by: kernel test robot
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 0f863fda56e6..dfade66bab73 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1704,8 +1704,6 @@ static struct device *create_namespace_pmem(struct
nd_region *nd_region,
res->flags = IORESOURCE_MEM;
for (i = 0; i < nd_region->ndr_mappings; i++) {
- uuid_t uuid;
-
nsl_get_uuid(ndd, nd_label, );
if (has_uuid_at_pos(nd_region, , cookie, i))
continue;
--
2.35.1
[PATCH v1 1/1] nvdimm/namespace: drop unneeded temporary variable in size_store()
Refactor size_store() in order to remove temporary variable on stack
by joining conditionals.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 3dae17c90e8c..0f863fda56e6 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -836,7 +836,6 @@ static ssize_t size_store(struct device *dev,
{
struct nd_region *nd_region = to_nd_region(dev->parent);
unsigned long long val;
- uuid_t **uuid = NULL;
int rc;
rc = kstrtoull(buf, 0, );
@@ -850,16 +849,12 @@ static ssize_t size_store(struct device *dev,
if (rc >= 0)
rc = nd_namespace_label_update(nd_region, dev);
- if (is_namespace_pmem(dev)) {
+ /* setting size zero == 'delete namespace' */
+ if (rc == 0 && val == 0 && is_namespace_pmem(dev)) {
struct nd_namespace_pmem *nspm = to_nd_namespace_pmem(dev);
- uuid = >uuid;
- }
-
- if (rc == 0 && val == 0 && uuid) {
- /* setting size zero == 'delete namespace' */
- kfree(*uuid);
- *uuid = NULL;
+ kfree(nspm->uuid);
+ nspm->uuid = NULL;
}
dev_dbg(dev, "%llx %s (%d)\n", val, rc < 0 ? "fail" : "success", rc);
--
2.35.1
[PATCH v1 1/1] nvdimm/namespace: return uuid_null only once in nd_dev_to_uuid()
Refactor nd_dev_to_uuid() in order to make code shorter and cleaner
by joining conditions and hence returning uuid_null only once.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index bf4f5c09d9b1..3dae17c90e8c 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -170,15 +170,12 @@ EXPORT_SYMBOL(nvdimm_namespace_disk_name);
const uuid_t *nd_dev_to_uuid(struct device *dev)
{
- if (!dev)
- return _null;
-
- if (is_namespace_pmem(dev)) {
+ if (dev && is_namespace_pmem(dev)) {
struct nd_namespace_pmem *nspm = to_nd_namespace_pmem(dev);
return nspm->uuid;
- } else
- return _null;
+ }
+ return _null;
}
EXPORT_SYMBOL(nd_dev_to_uuid);
--
2.35.1
Re: [PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper
On Wed, Mar 02, 2022 at 05:36:20PM +0100, Rafael J. Wysocki wrote: > On Wed, Mar 2, 2022 at 4:50 PM Andy Shevchenko > wrote: > > On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > > > Since we got list_entry_is_head() helper in the generic header, > > > we may switch the ACPI modules to use it. This eliminates the > > > need in additional variable. In some cases it reduces critical > > > sections as well. > > > > Besides the work required in a couple of cases (LKP) there is an > > ongoing discussion about list loops (and this particular API). > > > > Rafael, what do you think is the best course of action here? > > I think the current approach is to do the opposite of what this patch > is attempting to do: avoid using the list iterator outside of the > loop. OK, let's drop this change. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper
On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > Since we got list_entry_is_head() helper in the generic header, > we may switch the ACPI modules to use it. This eliminates the > need in additional variable. In some cases it reduces critical > sections as well. Besides the work required in a couple of cases (LKP) there is an ongoing discussion about list loops (and this particular API). Rafael, what do you think is the best course of action here? -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper
Since we got list_entry_is_head() helper in the generic header,
we may switch the ACPI modules to use it. This eliminates the
need in additional variable. In some cases it reduces critical
sections as well.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/acpi_ipmi.c | 16 ++--
drivers/acpi/glue.c | 8 +++-
drivers/acpi/nfit/core.c | 12 +++-
drivers/acpi/nfit/mce.c | 4 +---
drivers/acpi/resource.c | 9 +++--
drivers/acpi/utils.c | 7 ++-
6 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index a5fe2926bf50..f9e56138f8d1 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -354,27 +354,26 @@ static void ipmi_cancel_tx_msg(struct acpi_ipmi_device
*ipmi,
struct acpi_ipmi_msg *msg)
{
struct acpi_ipmi_msg *tx_msg, *temp;
- bool msg_found = false;
unsigned long flags;
spin_lock_irqsave(>tx_msg_lock, flags);
list_for_each_entry_safe(tx_msg, temp, >tx_msg_list, head) {
if (msg == tx_msg) {
- msg_found = true;
list_del(_msg->head);
break;
}
}
spin_unlock_irqrestore(>tx_msg_lock, flags);
- if (msg_found)
- acpi_ipmi_msg_put(tx_msg);
+ if (list_entry_is_head(tx_msg, >tx_msg_list, head)
+ return;
+
+ acpi_ipmi_msg_put(tx_msg);
}
static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
{
struct acpi_ipmi_device *ipmi_device = user_msg_data;
- bool msg_found = false;
struct acpi_ipmi_msg *tx_msg, *temp;
struct device *dev = ipmi_device->dev;
unsigned long flags;
@@ -389,14 +388,13 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg,
void *user_msg_data)
spin_lock_irqsave(_device->tx_msg_lock, flags);
list_for_each_entry_safe(tx_msg, temp, _device->tx_msg_list, head)
{
if (msg->msgid == tx_msg->tx_msgid) {
- msg_found = true;
list_del(_msg->head);
break;
}
}
spin_unlock_irqrestore(_device->tx_msg_lock, flags);
- if (!msg_found) {
+ if (list_entry_is_head(tx_msg, _device->tx_msg_list, head)) {
dev_warn(dev,
"Unexpected response (msg id %ld) is returned.\n",
msg->msgid);
@@ -483,13 +481,11 @@ static void ipmi_register_bmc(int iface, struct device
*dev)
static void ipmi_bmc_gone(int iface)
{
struct acpi_ipmi_device *ipmi_device, *temp;
- bool dev_found = false;
mutex_lock(_data.ipmi_lock);
list_for_each_entry_safe(ipmi_device, temp,
_data.ipmi_devices, head) {
if (ipmi_device->ipmi_ifnum != iface) {
- dev_found = true;
__ipmi_dev_kill(ipmi_device);
break;
}
@@ -500,7 +496,7 @@ static void ipmi_bmc_gone(int iface)
struct acpi_ipmi_device, head);
mutex_unlock(_data.ipmi_lock);
- if (dev_found) {
+ if (!list_entry_is_head(ipmi_device, _data.ipmi_devices, head)) {
ipmi_flush_tx_msg(ipmi_device);
acpi_ipmi_dev_put(ipmi_device);
}
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index ef104809f27b..ffc0b3ee190b 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -61,17 +61,15 @@ EXPORT_SYMBOL_GPL(unregister_acpi_bus_type);
static struct acpi_bus_type *acpi_get_bus_type(struct device *dev)
{
- struct acpi_bus_type *tmp, *ret = NULL;
+ struct acpi_bus_type *tmp;
down_read(_type_sem);
list_for_each_entry(tmp, _type_list, list) {
- if (tmp->match(dev)) {
- ret = tmp;
+ if (tmp->match(dev))
break;
- }
}
up_read(_type_sem);
- return ret;
+ return list_entry_is_head(tmp, _type_list, list) ? NULL : tmp;
}
#define FIND_CHILD_MIN_SCORE 1
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index e5d7f2bda13f..b31c16e5e42c 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1076,8 +1076,8 @@ static void nfit_mem_init_bdw(struct acpi_nfit_desc
*acpi_desc,
static int __nfit_mem_init(struct acpi_nfit_desc *acpi_desc,
struct acpi_nfit_system_address *spa)
{
- struct nfit_mem *nfit_mem, *found;
struct nfit_memdev *nfit_memdev;
+ struct nfit_mem *nfit_mem;
int type = spa ? nfit_spa_type(spa) : 0;
switch (type) {
@@ -1106,19 +1106,13 @@ static int __nfit_mem_init(struct acpi_nfit_desc
*acpi_desc,
[PATCH v1 1/1] ACPI: NFIT: Import GUID before use
Strictly speaking the comparison between guid_t and raw buffer
is not correct. Import GUID to variable of guid_t type and then
compare.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/nfit/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 7dd80acf92c7..e5d7f2bda13f 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -678,10 +678,12 @@ static const char *spa_type_name(u16 type)
int nfit_spa_type(struct acpi_nfit_system_address *spa)
{
+ guid_t guid;
int i;
+ import_guid(, spa->range_guid);
for (i = 0; i < NFIT_UUID_MAX; i++)
- if (guid_equal(to_nfit_uuid(i), (guid_t *)>range_guid))
+ if (guid_equal(to_nfit_uuid(i), ))
return i;
return -1;
}
--
2.33.0
Re: [PATCH V7 08/18] x86/entry: Preserve PKRS MSR across exceptions
On 11/12/21 16:50, Ira Weiny wrote: On Tue, Aug 03, 2021 at 09:32:21PM -0700, 'Ira Weiny' wrote: From: Ira Weiny The PKRS MSR is not managed by XSAVE. It is preserved through a context switch but this support leaves exception handling code open to memory accesses during exceptions. 2 possible places for preserving this state were considered, irqentry_state_t or pt_regs.[1] pt_regs was much more complicated and was potentially fraught with unintended consequences.[2] However, Andy came up with a way to hide additional values on the stack which could be accessed as "extended_pt_regs".[3] Andy, I'm preparing to send V8 of this PKS work. But I have not seen any feed back since I originally implemented this in V4[1]. Does this meets your expectations? Are there any issues you can see with this code? I think I'm generally okay with the approach to allocating space. All of Thomas' comments still apply, though. (Sorry, I'm horribly behind.)
[PATCH v2 2/2] spi: Avoid undefined behaviour when counting unused native CSs
ffz(), that has been used to count unused native CSs,
might cause undefined behaviour when called against ~0U.
To fix that, open code it with ffs(~value) - 1.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
v2: decoded UB abbreviation (Mark)
drivers/spi/spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9c3730a9f7d5..01f95bee2ac8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2609,7 +2609,7 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
native_cs_mask |= BIT(i);
}
- ctlr->unused_native_cs = ffz(native_cs_mask);
+ ctlr->unused_native_cs = ffs(~native_cs_mask) - 1;
if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
--
2.30.2
[PATCH v2 1/2] spi: Allow to have all native CSs in use along with GPIOs
The commit 7d93aecdb58d ("spi: Add generic support for unused native cs
with cs-gpios") excludes the valid case for the controllers that doesn't
need to switch native CS in order to perform the transfer, i.e. when
0 native
... ...
- 1 native
GPIO
+ 1 GPIO
... ...
where defines maximum of native CSs supported by the controller.
To allow this, bail out from spi_get_gpio_descs() conditionally for
the controllers which explicitly marked with SPI_MASTER_GPIO_SS.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
v2: no changes
drivers/spi/spi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 36c46feab6d4..9c3730a9f7d5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2610,8 +2610,9 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
}
ctlr->unused_native_cs = ffz(native_cs_mask);
- if (num_cs_gpios && ctlr->max_native_cs &&
- ctlr->unused_native_cs >= ctlr->max_native_cs) {
+
+ if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
+ ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
dev_err(dev, "No unused native chip select available\n");
return -EINVAL;
}
--
2.30.2
[PATCH v2 1/1] spi: Make error handling of gpiod_count() call cleaner
Each time we call spi_get_gpio_descs() the num_chipselect is overwritten
either by new value or by the old one. This is an extra operation in case
gpiod_count() returns an error. Besides that it slashes the error handling
of gpiod_count().
Refactor the code to make error handling of gpiod_count() call cleaner.
Note, that gpiod_count() never returns 0, take this into account as well.
Signed-off-by: Andy Shevchenko
---
v2: reformulated commit message and dropped Fixes tag
drivers/spi/spi.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 74b2b1dd358b..36c46feab6d4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2558,13 +2558,14 @@ static int spi_get_gpio_descs(struct spi_controller
*ctlr)
unsigned int num_cs_gpios = 0;
nb = gpiod_count(dev, "cs");
- ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
-
- /* No GPIOs at all is fine, else return the error */
- if (nb == 0 || nb == -ENOENT)
- return 0;
- else if (nb < 0)
+ if (nb < 0) {
+ /* No GPIOs at all is fine, else return the error */
+ if (nb == -ENOENT)
+ return 0;
return nb;
+ }
+
+ ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
cs = devm_kcalloc(dev, ctlr->num_chipselect, sizeof(*cs),
GFP_KERNEL);
--
2.30.2
Re: [PATCH v1 2/2] spi: Avoid potential UB when counting unused native CSs
On Tue, Apr 20, 2021 at 03:56:16PM +0100, Mark Brown wrote: > On Tue, Apr 20, 2021 at 05:10:04PM +0300, Andy Shevchenko wrote: > > ffz(), that has been used to count unused native CSs, might produce UB > > Bit of an IA there... UB -- undefined behaviour. I'll decode it. Should I decode CS as well? -- With Best Regards, Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 05:37:27PM +0300, Sakari Ailus wrote: > On Tue, Apr 20, 2021 at 02:55:33PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 20, 2021 at 01:56:40PM +0300, Sakari Ailus wrote: > > > On Tue, Apr 20, 2021 at 06:34:26PM +0800, Bingbu Cao wrote: > > > > On 4/20/21 6:20 PM, Andy Shevchenko wrote: > > > > > On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote: > > > > ... > > > > > > > This misses the changelog from v1 followed by the explanation why > > > > > resent. > > > > > > > > > I noticed there was a typo in the recipient list: > > > > stable.vger.kernel.org -> sta...@vger.kernel.org > > > > > > > > no code change for resent. > > > > > > When you're submitting a patch and want it reach the stable kernels, > > > you'll > > > need to add a Cc tag: > > > > > > Cc: sta...@vger.kernel.org > > > > > > But not actually add the address to cc. I dropped stable@vger address from > > > distribution. > > > > Does it really matter? > > Usually aligning what you're doing with > Documentation/process/submitting-patches.rst is not a bad idea. True, my point is that technically both ways will give the same result, no? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] spi: Don't overwrite num_chipselect with error code
On Tue, Apr 20, 2021 at 4:19 PM Andy Shevchenko
wrote:
>
> The code currently organized in a way that num_chipselect is overwritten
> each time we call spi_get_gpio_descs(). It might be potentially dangerous
> in case when the gpiod_count() returns an error code.
>
> Note, that gpiod_count() never returns 0, take this into account as well.
>
> Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
It doesn't fix anything. I missed the max_t(int).
In any case it makes error handling cleaner, so I'll reformulate the
commit message in v2 and drop Fixes tag.
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/2] spi: Allow to have all native CSs in use along with GPIOs
The commit 7d93aecdb58d ("spi: Add generic support for unused native cs
with cs-gpios") excludes the valid case for the controllers that doesn't
need to switch native CS in order to perform the transfer, i.e. when
0 native
... ...
- 1 native
GPIO
+ 1 GPIO
... ...
where defines maximum of native CSs supported by the controller.
To allow this, bail out from spi_get_gpio_descs() conditionally for
the controllers which explicitly marked with SPI_MASTER_GPIO_SS.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 36c46feab6d4..9c3730a9f7d5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2610,8 +2610,9 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
}
ctlr->unused_native_cs = ffz(native_cs_mask);
- if (num_cs_gpios && ctlr->max_native_cs &&
- ctlr->unused_native_cs >= ctlr->max_native_cs) {
+
+ if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
+ ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
dev_err(dev, "No unused native chip select available\n");
return -EINVAL;
}
--
2.30.2
[PATCH v1 2/2] spi: Avoid potential UB when counting unused native CSs
ffz(), that has been used to count unused native CSs, might produce UB
when called against ~0U. To fix that, open code it with ffs(~value) - 1.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9c3730a9f7d5..01f95bee2ac8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2609,7 +2609,7 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
native_cs_mask |= BIT(i);
}
- ctlr->unused_native_cs = ffz(native_cs_mask);
+ ctlr->unused_native_cs = ffs(~native_cs_mask) - 1;
if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
--
2.30.2
[PATCH v1 1/1] spi: Don't overwrite num_chipselect with error code
The code currently organized in a way that num_chipselect is overwritten
each time we call spi_get_gpio_descs(). It might be potentially dangerous
in case when the gpiod_count() returns an error code.
Note, that gpiod_count() never returns 0, take this into account as well.
Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 74b2b1dd358b..36c46feab6d4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2558,13 +2558,14 @@ static int spi_get_gpio_descs(struct spi_controller
*ctlr)
unsigned int num_cs_gpios = 0;
nb = gpiod_count(dev, "cs");
- ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
-
- /* No GPIOs at all is fine, else return the error */
- if (nb == 0 || nb == -ENOENT)
- return 0;
- else if (nb < 0)
+ if (nb < 0) {
+ /* No GPIOs at all is fine, else return the error */
+ if (nb == -ENOENT)
+ return 0;
return nb;
+ }
+
+ ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
cs = devm_kcalloc(dev, ctlr->num_chipselect, sizeof(*cs),
GFP_KERNEL);
--
2.30.2
[PATCH v1 1/1] spi: Rename enable1 to activate in spi_set_cs()
The enable1 is confusing name. Change it to clearly show what is
the intention behind it. No functional changes.
Fixes: 25093bdeb6bc ("spi: implement SW control for CS times")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b08efe88ccd6..74b2b1dd358b 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -797,7 +797,7 @@ int spi_register_board_info(struct spi_board_info const
*info, unsigned n)
static void spi_set_cs(struct spi_device *spi, bool enable)
{
- bool enable1 = enable;
+ bool activate = enable;
/*
* Avoid calling into the driver (or doing delays) if the chip select
@@ -812,7 +812,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
!spi->controller->set_cs_timing) {
- if (enable1)
+ if (activate)
spi_delay_exec(>controller->cs_setup, NULL);
else
spi_delay_exec(>controller->cs_hold, NULL);
@@ -825,8 +825,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
if (!(spi->mode & SPI_NO_CS)) {
if (spi->cs_gpiod)
/* polarity handled by gpiolib */
- gpiod_set_value_cansleep(spi->cs_gpiod,
-enable1);
+ gpiod_set_value_cansleep(spi->cs_gpiod,
activate);
else
/*
* invert the enable line, as active low is
@@ -844,7 +843,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
!spi->controller->set_cs_timing) {
- if (!enable1)
+ if (!activate)
spi_delay_exec(>controller->cs_inactive, NULL);
}
}
--
2.30.2
Re: linux-next: Tree for Apr 20
On Tue, Apr 20, 2021 at 03:19:39PM +0300, Andy Shevchenko wrote: > On Tue, Apr 20, 2021 at 03:02:51PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 20, 2021 at 07:47:18PM +1000, Stephen Rothwell wrote: ... > > I have full of build warnings / errors in x86 and iommu Found the culprit -- it was uncleaned stuff from the other build in the source tree. So, it was only me who experienced that :-) -- With Best Regards, Andy Shevchenko
Re: linux-next: Tree for Apr 20
On Tue, Apr 20, 2021 at 03:02:51PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 20, 2021 at 07:47:18PM +1000, Stephen Rothwell wrote:
> > Hi all,
> >
> > Changes since 20210419:
> >
> > The powerpc tree lost its build failure.
> >
> > The ftrace tree gained a conflict against the bpf-next tree.
> >
> > Non-merge commits (relative to Linus' tree): 12917
> > 11294 files changed, 619161 insertions(+), 276245 deletions(-)
> >
> >
> >
> > I have created today's linux-next tree at
> > git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > (patches at http://www.kernel.org/pub/linux/kernel/next/ ). If you
> > are tracking the linux-next tree using git, you should not use "git pull"
> > to do so as that will try to merge the new linux-next release with the
> > old one. You should use "git fetch" and checkout or reset to the new
> > master.
> >
> > You can see which trees have been included by looking in the Next/Trees
> > file in the source. There are also quilt-import.log and merge.log
> > files in the Next directory. Between each merge, the tree was built
> > with a ppc64_defconfig for powerpc, an allmodconfig for x86_64, a
> > multi_v7_defconfig for arm and a native build of tools/perf. After
> > the final fixups (if any), I do an x86_64 modules_install followed by
> > builds for x86_64 allnoconfig, powerpc allnoconfig (32 and 64 bit),
> > ppc44x_defconfig, allyesconfig and pseries_le_defconfig and i386, sparc
> > and sparc64 defconfig and htmldocs. And finally, a simple boot test
> > of the powerpc pseries_le_defconfig kernel in qemu (with and without
> > kvm enabled).
> >
> > Below is a summary of the state of the merge.
> >
> > I am currently merging 340 trees (counting Linus' and 89 trees of bug
> > fix patches pending for the current merge release).
> >
> > Stats about the size of the tree over time can be seen at
> > http://neuling.org/linux-next-size.html .
> >
> > Status of my local build tests will be at
> > http://kisskb.ellerman.id.au/linux-next . If maintainers want to give
> > advice about cross compilers/configs that work, we are always open to add
> > more builds.
> >
> > Thanks to Randy Dunlap for doing many randconfig builds. And to Paul
> > Gortmaker for triage and bug fixes.
>
> I have full of build warnings / errors in x86 and iommu
>
> X86:
>
> arch/x86/include/asm/string_64.h:14:14: warning: conflicting types for
> built-in function ‘memcpy’; expected ‘void *(void *, const void *, long
> unsigned int)’ [-Wbuiltin-declaration-mismatch]
>14 | extern void *memcpy(void *to, const void *from, size_t len);
> | ^~
> arch/x86/include/asm/string_64.h:7:1: note: ‘memcpy’ is declared in header
> ‘’
> 6 | #include
> +++ |+#include
>
> And so on for standard string function definitions.
>
> IOMMU:
>
> drivers/iommu/amd/io_pgtable.c: In function ‘v1_alloc_pgtable’:
> drivers/iommu/amd/io_pgtable.c:551:32: error: assignment to ‘size_t
> (*)(struct io_pgtable_ops *, long unsigned int, size_t, struct
> iommu_iotlb_gather *)’ {aka ‘unsigned int (*)(struct io_pgtable_ops *, long
> unsigned int, unsigned int, struct iommu_iotlb_gather *)’} from
> incompatible pointer type ‘long unsigned int (*)(struct io_pgtable_ops *,
> long unsigned int, size_t, struct iommu_iotlb_gather *)’ {aka ‘long
> unsigned int (*)(struct io_pgtable_ops *, long unsigned int, unsigned int,
> struct iommu_iotlb_gather *)’} [-Werror=incompatible-pointer-types]
> 551 | pgtable->iop.ops.unmap= iommu_v1_unmap_page;
> |^
> cc1: some warnings being treated as errors
>
> Is it only me?
Okay, there is another bug and it seems compiler related:
net/socket.c:2320:3: note: in expansion of macro ‘BUILD_BUG_ON’
2320 | BUILD_BUG_ON(sizeof(struct cmsghdr) !=
| ^~~~
% gcc --version
gcc (Debian 10.2.1-6) 10.2.1 20210110
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Tue, Apr 20, 2021 at 2:36 PM Tomas Melin wrote:
> On 4/20/21 1:47 PM, Andy Shevchenko wrote:
> > On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin
> > wrote:
...
> >>>> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> >>>> +indio_dev->masklength) {
> >>>> + ret = sca3300_read_reg(data,
> >>>> sca3300_channels[bit].address,
> >>>> + );
> >>>> + if (ret) {
> >>>> + dev_err(>spi->dev,
> >>>> + "failed to read register, error: %d\n",
> >>>> ret);
> >>>> + goto out;
> >>> Does it mean interrupt is handled in this case?
> >>> Perhaps a comment why it's okay to consider so?
> >> IRQ_HANDLED seemed more correct than IRQ_NONE.
> > Why? Care to explain?
>
> Thinking that IRQ was for the device and it was indeed handled. There
> were errors when handling
>
> it, but it was handled as much as possible.
>
> >
> >> Or did You have some
> >> other option in mind?
> >>
> >> How about something like:
> >>
> >> /* handled with errors */
> > But what if this is the very first interrupt (bit in the loop) that
> > failed? What about the rest?
>
> Aah, right. Other option could be to simply continue loop and set 'val'
> to e.g. 0 for
>
> readings with errors. But perhaps it is after all better to bail out,
> and only for cases
>
> when _all_ data is reliable, it is pushed to buffers(?)
>
> Comes to mind that perhaps better to have error message in this irq
> handler as
>
> dev_err_ratelimited(), to avoid possible flooding.
>
>
> So to conclude, proposing:
>
> *change to dev_err_ratelimited()
>
> * comment goto:
>
> /* handled, but bailing out this round due to errors */
>
> Would this be OK?
Sounds like a plan!
> >> goto out;
> >>
> >>>> + }
> >>>> + data->scan.channels[i++] = val;
> >>>> + }
--
With Best Regards,
Andy Shevchenko
Re: linux-next: Tree for Apr 20
On Tue, Apr 20, 2021 at 07:47:18PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20210419:
>
> The powerpc tree lost its build failure.
>
> The ftrace tree gained a conflict against the bpf-next tree.
>
> Non-merge commits (relative to Linus' tree): 12917
> 11294 files changed, 619161 insertions(+), 276245 deletions(-)
>
>
>
> I have created today's linux-next tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> (patches at http://www.kernel.org/pub/linux/kernel/next/ ). If you
> are tracking the linux-next tree using git, you should not use "git pull"
> to do so as that will try to merge the new linux-next release with the
> old one. You should use "git fetch" and checkout or reset to the new
> master.
>
> You can see which trees have been included by looking in the Next/Trees
> file in the source. There are also quilt-import.log and merge.log
> files in the Next directory. Between each merge, the tree was built
> with a ppc64_defconfig for powerpc, an allmodconfig for x86_64, a
> multi_v7_defconfig for arm and a native build of tools/perf. After
> the final fixups (if any), I do an x86_64 modules_install followed by
> builds for x86_64 allnoconfig, powerpc allnoconfig (32 and 64 bit),
> ppc44x_defconfig, allyesconfig and pseries_le_defconfig and i386, sparc
> and sparc64 defconfig and htmldocs. And finally, a simple boot test
> of the powerpc pseries_le_defconfig kernel in qemu (with and without
> kvm enabled).
>
> Below is a summary of the state of the merge.
>
> I am currently merging 340 trees (counting Linus' and 89 trees of bug
> fix patches pending for the current merge release).
>
> Stats about the size of the tree over time can be seen at
> http://neuling.org/linux-next-size.html .
>
> Status of my local build tests will be at
> http://kisskb.ellerman.id.au/linux-next . If maintainers want to give
> advice about cross compilers/configs that work, we are always open to add
> more builds.
>
> Thanks to Randy Dunlap for doing many randconfig builds. And to Paul
> Gortmaker for triage and bug fixes.
I have full of build warnings / errors in x86 and iommu
X86:
arch/x86/include/asm/string_64.h:14:14: warning: conflicting types for built-in
function ‘memcpy’; expected ‘void *(void *, const void *, long unsigned int)’
[-Wbuiltin-declaration-mismatch]
14 | extern void *memcpy(void *to, const void *from, size_t len);
| ^~
arch/x86/include/asm/string_64.h:7:1: note: ‘memcpy’ is declared in header
‘’
6 | #include
+++ |+#include
And so on for standard string function definitions.
IOMMU:
drivers/iommu/amd/io_pgtable.c: In function ‘v1_alloc_pgtable’:
drivers/iommu/amd/io_pgtable.c:551:32: error: assignment to ‘size_t (*)(struct
io_pgtable_ops *, long unsigned int, size_t, struct iommu_iotlb_gather *)’
{aka ‘unsigned int (*)(struct io_pgtable_ops *, long unsigned int, unsigned
int, struct iommu_iotlb_gather *)’} from incompatible pointer type ‘long
unsigned int (*)(struct io_pgtable_ops *, long unsigned int, size_t, struct
iommu_iotlb_gather *)’ {aka ‘long unsigned int (*)(struct io_pgtable_ops *,
long unsigned int, unsigned int, struct iommu_iotlb_gather *)’}
[-Werror=incompatible-pointer-types]
551 | pgtable->iop.ops.unmap= iommu_v1_unmap_page;
|^
cc1: some warnings being treated as errors
Is it only me?
--
With Best Regards,
Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 01:56:40PM +0300, Sakari Ailus wrote: > On Tue, Apr 20, 2021 at 06:34:26PM +0800, Bingbu Cao wrote: > > On 4/20/21 6:20 PM, Andy Shevchenko wrote: > > > On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote: ... > > > This misses the changelog from v1 followed by the explanation why resent. > > > > > I noticed there was a typo in the recipient list: > > stable.vger.kernel.org -> sta...@vger.kernel.org > > > > no code change for resent. > > When you're submitting a patch and want it reach the stable kernels, you'll > need to add a Cc tag: > > Cc: sta...@vger.kernel.org > > But not actually add the address to cc. I dropped stable@vger address from > distribution. Does it really matter? -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin wrote:
> On 4/19/21 4:55 PM, Andy Shevchenko wrote:
> > On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin wrote:
...
> >> +#define SCA3300_MASK_STATUSGENMASK(8, 0)
> >> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
> > This feels like an orphan. Shouldn't you move it closer to the group
> > of corresponding register / etc definition?
>
> Tried to group these in alphabetical order, but IIUC preference would be
> towards grouping
Yes, alphabetical is about header block, and definition should be
understandable and HW represented.
> according to how they are used? Would this be clearer and acceptable?
1) with some amendments, see below.
> 1)
>
> /* Device mode register */
> #define SCA3300_REG_MODE0xd
> #define SCA3300_VALUE_SW_RESET0x20
SCA3300_MODE_SW_RESET
> /* Last register in map */
> #define SCA3300_REG_SELBANK0x1f
>
> /* Device status and related mask */
> #define SCA3300_REG_STATUS0x6
> #define SCA3300_MASK_STATUSGENMASK(8, 0)
SCA3300_STATUS_MASK
and so on (I guess you got the pattern)
> /* Device ID */
> #define SCA3300_REG_WHOAMI0x10
> #define SCA3300_VALUE_DEVICE_ID0x51
>
> /* Device return status and mask */
> #define SCA3300_VALUE_RS_ERROR0x3
> #define SCA3300_MASK_RS_STATUSGENMASK(1, 0)
...
> >> + * @txbuf: Transmit buffer
> >> + * @rxbuf: Receive buffer
> > Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> Good point, I will add alignment.
Move them to the end of the structure to save few bytes,
...
> >> + sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
> > Seems you ignored my comment. What is this 0x0? What is the meaning of it?
> > Same for all the rest magic numbers in the code.
>
> Sorry, not ignored but will remove this redundant 0x0 for next round.
Maybe it's not redundant after all (I noticed other magic numbers in
the same position)? Please, comment your intention case-by-case.
...
> >> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> +indio_dev->masklength) {
> >> + ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> >> + );
> >> + if (ret) {
> >> + dev_err(>spi->dev,
> >> + "failed to read register, error: %d\n",
> >> ret);
> >> + goto out;
> > Does it mean interrupt is handled in this case?
> > Perhaps a comment why it's okay to consider so?
>
> IRQ_HANDLED seemed more correct than IRQ_NONE.
Why? Care to explain?
> Or did You have some
> other option in mind?
>
> How about something like:
>
> /* handled with errors */
But what if this is the very first interrupt (bit in the loop) that
failed? What about the rest?
> goto out;
>
> >> + }
> >> + data->scan.channels[i++] = val;
> >> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] optee: use export_uuid() to copy client UUID
On Tue, Apr 20, 2021 at 10:25:47AM +0200, Jens Wiklander wrote:
> Prior to this patch optee_open_session() was making assumptions about
> the internal format of uuid_t by casting a memory location in a
> parameter struct to uuid_t *. Fix this using export_uuid() to get a well
> defined binary representation and also add an octets field in struct
> optee_msg_param in order to avoid casting.
Wonderful! Thanks for fixing this!
Reviewed-by: Andy Shevchenko
A bit of off-topic, have you know by any chance who may consider applying this
one?
https://lore.kernel.org/linux-mips/20210121183741.45333-1-andriy.shevche...@linux.intel.com/
> Fixes: c5b4312bea5d ("tee: optee: Add support for session login client UUID
> generation")
> Suggested-by: Andy Shevchenko
> Signed-off-by: Jens Wiklander
> ---
> drivers/tee/optee/call.c | 6 --
> drivers/tee/optee/optee_msg.h | 6 --
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 7a77e375b503..6b52f0c526ba 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -216,6 +216,7 @@ int optee_open_session(struct tee_context *ctx,
> struct optee_msg_arg *msg_arg;
> phys_addr_t msg_parg;
> struct optee_session *sess = NULL;
> + uuid_t client_uuid;
>
> /* +2 for the meta parameters added below */
> shm = get_msg_arg(ctx, arg->num_params + 2, _arg, _parg);
> @@ -236,10 +237,11 @@ int optee_open_session(struct tee_context *ctx,
> memcpy(_arg->params[0].u.value, arg->uuid, sizeof(arg->uuid));
> msg_arg->params[1].u.value.c = arg->clnt_login;
>
> - rc = tee_session_calc_client_uuid((uuid_t *)_arg->params[1].u.value,
> - arg->clnt_login, arg->clnt_uuid);
> + rc = tee_session_calc_client_uuid(_uuid, arg->clnt_login,
> + arg->clnt_uuid);
> if (rc)
> goto out;
> + export_uuid(msg_arg->params[1].u.octets, _uuid);
>
> rc = optee_to_msg_param(msg_arg->params + 2, arg->num_params, param);
> if (rc)
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 81ff593ac4ec..e3d72d09c484 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -9,7 +9,7 @@
> #include
>
> /*
> - * This file defines the OP-TEE message protocol used to communicate
> + * This file defines the OP-TEE message protocol (ABI) used to communicate
> * with an instance of OP-TEE running in secure world.
> *
> * This file is divided into two sections.
> @@ -144,9 +144,10 @@ struct optee_msg_param_value {
> * @tmem:parameter by temporary memory reference
> * @rmem:parameter by registered memory reference
> * @value: parameter by opaque value
> + * @octets: parameter by octet string
> *
> * @attr & OPTEE_MSG_ATTR_TYPE_MASK indicates if tmem, rmem or value is used
> in
> - * the union. OPTEE_MSG_ATTR_TYPE_VALUE_* indicates value,
> + * the union. OPTEE_MSG_ATTR_TYPE_VALUE_* indicates value or octets,
> * OPTEE_MSG_ATTR_TYPE_TMEM_* indicates @tmem and
> * OPTEE_MSG_ATTR_TYPE_RMEM_* indicates @rmem,
> * OPTEE_MSG_ATTR_TYPE_NONE indicates that none of the members are used.
> @@ -157,6 +158,7 @@ struct optee_msg_param {
> struct optee_msg_param_tmem tmem;
> struct optee_msg_param_rmem rmem;
> struct optee_msg_param_value value;
> + u8 octets[24];
> } u;
> };
>
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote: > Intel IPU(Image Processing Unit) has its own (IO)MMU hardware, > The IPU driver allocates its own page table that is not mapped > via the DMA, and thus the Intel IOMMU driver blocks access giving > this error: > > DMAR: DRHD: handling fault status reg 3 > DMAR: [DMA Read] Request device [00:05.0] PASID > fault addr 76406000 [fault reason 06] PTE Read access is not set > > As IPU is not an external facing device which is not risky, so use > IOMMU passthrough mode for Intel IPUs. I'm wondering if IPU MMU should be described properly in the DMAR table. -- With Best Regards, Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote:
> Intel IPU(Image Processing Unit) has its own (IO)MMU hardware,
> The IPU driver allocates its own page table that is not mapped
> via the DMA, and thus the Intel IOMMU driver blocks access giving
> this error:
>
> DMAR: DRHD: handling fault status reg 3
> DMAR: [DMA Read] Request device [00:05.0] PASID
> fault addr 76406000 [fault reason 06] PTE Read access is not set
>
> As IPU is not an external facing device which is not risky, so use
> IOMMU passthrough mode for Intel IPUs.
>
> Fixes: 26f5689592e2 ("media: staging/intel-ipu3: mmu: Implement driver")
> Signed-off-by: Bingbu Cao
> ---
> drivers/iommu/intel/iommu.c | 29 +
This misses the changelog from v1 followed by the explanation why resent.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 0/2] bitmap_parselist: support 'all' semantics
On Mon, Apr 19, 2021 at 05:01:29PM -0700, Yury Norov wrote: > RCU code supports a special group 'all' which selects all bits in a bitmap. > We have recently added 'N' extension for bitmap parse, so that '0-N' would > have exactly the same meaning as 'all'. But because the 'all' is already > used by RCU, it would be reasonable to support it in core bitmap code as a > common and easy-readable alias for '0-N'. > > Moving the 'all' support to core bitmap code adds another level of > flexibility for system configuration by supporting patterns. For example, > every second bit in cpumask may be selected like this: > isolcpus=all:1/2 After addressing a couple of nit-picks, Reviewed-by: Andy Shevchenko > Yury Norov (2): > bitmap_parse: support 'all' semantics > rcu/tree_plugin: don't handle the case of 'all' CPU range > > Documentation/admin-guide/kernel-parameters.rst | 5 + > kernel/rcu/tree_plugin.h| 9 +++-- > lib/bitmap.c| 9 + > lib/test_bitmap.c | 8 +++- > 4 files changed, 24 insertions(+), 7 deletions(-) > > -- > 2.25.1 > -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] rcu/tree_plugin: don't handle the case of 'all' CPU range
On Mon, Apr 19, 2021 at 05:01:31PM -0700, Yury Norov wrote: > The 'all' semantics is now supported by the bitmap_parselist() so we can > drop supporting it as a special case in RCU code. This patch does not > add any functional changes for existing users. > - if (!strcasecmp(str, "all"))/* legacy: use "0-N" instead */ Perhaps move comment as well to new location. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] bitmap_parse: support 'all' semantics
On Mon, Apr 19, 2021 at 05:01:30PM -0700, Yury Norov wrote:
> RCU code supports an 'all' group as a special case when parsing
> rcu_nocbs parameter. This patch moves the 'all' support to the core
> bitmap_parse code, so that all bitmap users can enjoy this extension.
>
> Moving 'all' parsing to a bitmap_parse level, also allows users to
> pass patterns together with 'all' in regular group:pattern format
...
> {0, "0-31:1/3,1-31:1/3,2-31:1/3", [8 * step], 32, 0},
> {0, "1-10:8/12,8-31:24/29,0-31:0/3",[9 * step], 32, 0},
>
> + {0, "all",[8 * step], 32, 0},
> + {0, "0, 1, all, ", [8 * step], 32, 0},
> + {0, "all:1/2",[4 * step], 32, 0},
> + {0, "ALL:1/2",[4 * step], 32, 0},
> + {-EINVAL, "al", NULL, 8, 0},
> + {-EINVAL, "alll", NULL, 8, 0},
> +
Looking at the below hunk it seems like the two above should be actually placed
there.
> {-EINVAL, "-1", NULL, 8, 0},
> {-EINVAL, "-0", NULL, 8, 0},
> {-EINVAL, "10-1", NULL, 8, 0},
> @@ -384,7 +391,6 @@ static const struct test_bitmap_parselist
> parselist_tests[] __initconst = {
> {-EINVAL, "a-31:10/1", NULL, 8, 0},
> {-EINVAL, "0-31:a/1", NULL, 8, 0},
> {-EINVAL, "0-\n", NULL, 8, 0},
> -
Otherwise this change doesn't belong to the series.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] pinctrl: core: Show pin numbers for the controllers with base = 0
On Tue, Apr 20, 2021 at 12:32:18AM -0700, Drew Fustini wrote:
> On Thu, Apr 15, 2021 at 04:03:56PM +0300, Andy Shevchenko wrote:
> > The commit f1b206cf7c57 ("pinctrl: core: print gpio in pins debugfs file")
> > enabled GPIO pin number and label in debugfs for pin controller. However,
> > it limited that feature to the chips where base is positive number. This,
> > in particular, excluded chips where base is 0 for the historical or backward
> > compatibility reasons. Refactor the code to include the latter as well.
...
> > - chip = gpio_to_chip(gpio_num);
> > - if (chip && chip->gpiodev && chip->gpiodev->base)
> > - seq_printf(s, "%u:%s ", gpio_num -
> > - chip->gpiodev->base, chip->label);
> > + if (gpio_num >= 0)
> > + chip = gpio_to_chip(gpio_num);
> > + else
> > + chip = NULL;
> > + if (chip)
> > + seq_printf(s, "%u:%s ", gpio_num - chip->gpiodev->base,
> > chip->label);
> > else
> > seq_puts(s, "0:? ");
> Thank you, this makes sense to me. I had failed to consider what would
> happen when chip->gpiodev->base == 0.
If gpiodev->base == 0 it can happen only when
1) either base is 0 by the driver request
2) or it's a GPIO device which fits the (last) free slot in the number space
It can't be negative at all. So, it means whatever value is there it is always
valid.
> I have tested on the BeagleBone
> (AM3358) and the output works as expected.
Cool!
> /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# more pins
> registered pins: 142
> pin 0 (PIN0) 0:gpio-0-31 44e10800 0027 pinctrl-single
> pin 1 (PIN1) 1:gpio-0-31 44e10804 0027 pinctrl-single
> pin 2 (PIN2) 2:gpio-0-31 44e10808 0027 pinctrl-single
> pin 3 (PIN3) 3:gpio-0-31 44e1080c 0027 pinctrl-single
> pin 4 (PIN4) 4:gpio-0-31 44e10810 0027 pinctrl-single
> pin 5 (PIN5) 5:gpio-0-31 44e10814 0027 pinctrl-single
> pin 6 (PIN6) 6:gpio-0-31 44e10818 0027 pinctrl-single
> pin 7 (PIN7) 7:gpio-0-31 44e1081c 0027 pinctrl-single
> pin 8 (PIN8) 22:gpio-96-127 44e10820 0027 pinctrl-single
> pin 9 (PIN9) 23:gpio-96-127 44e10824 0037 pinctrl-single
> pin 10 (PIN10) 26:gpio-96-127 44e10828 0037 pinctrl-single
> pin 11 (PIN11) 27:gpio-96-127 44e1082c 0037 pinctrl-single
> pin 12 (PIN12) 12:gpio-0-31 44e10830 0037 pinctrl-single
> pin 13 (PIN13) 13:gpio-0-31 44e10834 0037 pinctrl-single
> pin 14 (PIN14) 14:gpio-0-31 44e10838 0037 pinctrl-single
> pin 15 (PIN15) 15:gpio-0-31 44e1083c 0037 pinctrl-single
> pin 16 (PIN16) 16:gpio-0-31 44e10840 0027 pinctrl-single
>
>
> Tested-by: Drew Fustini
> Reviewed-by: Drew Fustini
Thank you!
Linus, can it be applied now?
--
With Best Regards,
Andy Shevchenko
Re: [RFC Part2 PATCH 04/30] x86/mm: split the physmap when adding the page in RMP table
> On Apr 19, 2021, at 11:33 AM, Dave Hansen wrote: > > On 4/19/21 11:10 AM, Andy Lutomirski wrote: >> I’m confused by this scenario. This should only affect physical pages >> that are in the 2M area that contains guest memory. But, if we have a >> 2M direct map PMD entry that contains kernel data and guest private >> memory, we’re already in a situation in which the kernel touching >> that memory would machine check, right? > > Not machine check, but page fault. Do machine checks even play a > special role in SEV-SNP? I thought that was only TDX? Brain fart. > > My point was just that you can't _easily_ do the 2M->4k kernel mapping > demotion in a page fault handler, like I think Borislav was suggesting. We are certainly toast if this hits the stack. Or if it hits a page table or the GDT or IDT :). The latter delightful choices would be triple faults. I sure hope the code we use to split a mapping is properly NMI safe. > >> ISTM we should fully unmap any guest private page from the kernel and >> all host user pagetables before actually making it be a guest private >> page. > > Yes, that sounds attractive. Then, we'd actually know if the host > kernel was doing stray reads somehow because we'd get a fault there too.
Re: [RFC Part2 PATCH 04/30] x86/mm: split the physmap when adding the page in RMP table
> On Apr 19, 2021, at 10:58 AM, Dave Hansen wrote: > > On 4/19/21 10:46 AM, Brijesh Singh wrote: >> - guest wants to make gpa 0x1000 as a shared page. To support this, we >> need to psmash the large RMP entry into 512 4K entries. The psmash >> instruction breaks the large RMP entry into 512 4K entries without >> affecting the previous validation. Now the we need to force the host to >> use the 4K page level instead of the 2MB. >> >> To my understanding, Linux kernel fault handler does not build the page >> tables on demand for the kernel addresses. All kernel addresses are >> pre-mapped on the boot. Currently, I am proactively spitting the physmap >> to avoid running into situation where x86 page level is greater than the >> RMP page level. > > In other words, if the host maps guest memory with 2M mappings, the > guest can induce page faults in the host. The only way the host can > avoid this is to map everything with 4k mappings. > > If the host does not avoid this, it could end up in the situation where > it gets page faults on access to kernel data structures. Imagine if a > kernel stack page ended up in the same 2M mapping as a guest page. I > *think* the next write to the kernel stack would end up double-faulting. I’m confused by this scenario. This should only affect physical pages that are in the 2M area that contains guest memory. But, if we have a 2M direct map PMD entry that contains kernel data and guest private memory, we’re already in a situation in which the kernel touching that memory would machine check, right? ISTM we should fully unmap any guest private page from the kernel and all host user pagetables before actually making it be a guest private page.
Re: [PATCH 05/15] x86: Implement function_nocfi
> On Apr 19, 2021, at 8:26 AM, David Laight wrote: > > From: Andy Lutomirski >> Sent: 18 April 2021 01:12 > .. >> Slightly more complicated: >> >> struct opaque_symbol; >> extern struct opaque_symbol entry_SYSCALL_64; >> >> The opaque_symbol variant avoids any possible confusion over the weird >> status of arrays in C, and it's hard to misuse, since struct >> opaque_symbol is an incomplete type. > > Maybe: > > s/opaque_symbol/entry_SYSCALL_64/ > Cute. OTOH, I’m not sure whether that has much benefit, and having a single type for all of this allows it to be declared just once. I suppose the magic could be wrapped in a macro, though. >David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales)
Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values
On Mon, Apr 19, 2021 at 5:30 PM Jens Wiklander
wrote:
> On Mon, Apr 19, 2021 at 3:40 PM Andy Shevchenko
> wrote:
> > On Mon, Apr 19, 2021 at 4:30 PM Jens Wiklander
> > wrote:
> > > On Mon, Apr 19, 2021 at 2:01 PM Andy Shevchenko
> > > wrote:
> > > > On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> > > > > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> > > > > wrote:
> > > >
> > > > Thanks for review, my answer below.
> > > >
> > > > > > struct optee_msg_param_tmem tmem;
> > > > > > struct optee_msg_param_rmem rmem;
> > > > > > struct optee_msg_param_value value;
> > > > > > + uuid_t uuid;
> > > > >
> > > > > It's nice to get rid of the cast above, but I'm not that keen on the
> > > > > change in this struct. This file defines the ABI towards Secure world
> > > > > and adding dependencies on external complex types is a larger problem
> > > > > than the cast above in my opinion.
> > > >
> > > > I understand.
> > > >
> > > > So, the cast is simply wrong there. Can you add a comment above that
> > > > cast to
> > > > explain that and make it is marked as FIXME? Because there is no
> > > > guarantee that
> > > > internal Linux types can be 1:1 mapped to the ABI of something.
> > >
> > > We might as well fix it directly instead. How about storing the
> > > intermediate result in a proper uuid_t and then export it as:
> > > export_uuid((u8 *)_arg->params[1].u.uuid, );
> >
> > Still a casting here.
> > With u64 members you have a (potential) endianness issue (consider
> > BE-32 platform). Also you never know that a b c translates properly to
> > byte array.
> >
> > I would rather see a custom function
> >
> > optee_import_uuid(param, uuid_t *uuid)
> > {
> > u8 uuid_raw[UUID_SIZE];
> >
> > put_unaligned_le64(_raw[0], param.a); // not sure about endianness
> > put_unaligned_le64(_raw[0], param.b); // ditto
>
> I believe it's a memcpy() we want then, since UUIDs are supposed to be
> transmitted using a big endian memory pattern.
> We should perhaps add
> u8 octets[24];
> to that union. Then should the result be well defined using export_uuid().
Right, if you do that, it would be wonderful!
> > import_uuid();
> > }
> >
> > > > What you need, perhaps, is a middle layer function that will copy u64
> > > > data
> > > > to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
> > > > variants are not in use?
> > >
> > > Does it make any difference? The file isn't shared with user space and
> > > I need to sync the file manually anyway since OP-TEE doesn't have the
> > > same include files.
> >
> > Yes. It gives a hint that these are ABI (that's why I felt free to add
> > a member to the union. I have no idea that's an ABI). Optionally a
> > comment suggesting that.
>
> It does say that it defines a protocol at the beginning of the file, I
> can add ABI too if you think that helps.
I read the structure definition, perhaps some clarification on a data
type level would be nice.
Thanks!
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 5:15 PM Geert Uytterhoeven wrote: > On Mon, Apr 19, 2021 at 3:58 PM Andy Shevchenko > wrote: > Please tell me how this driver will be probed when CONFIG_ACPI > is disabled (it cannot, as nothing instantiates platform devices of the > right type, so there is no reason to bother the user with a question about > this driver when configuring his kernel). Go ahead with it in v2. I'll not block you. -- With Best Regards, Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 5:18 PM Geert Uytterhoeven wrote: > On Mon, Apr 19, 2021 at 4:14 PM Andy Shevchenko > wrote: > > On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko > > wrote: > > > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven > > > wrote: > > > > > > > In any case it's not true. We have the platform drivers w/o legacy > > > > > users that are not dependent on OF. > > > > > > > > Example? ;-) > > > > > > i2c-owl.c > > > > In case you want more > > sound/sparc/amd7930.c > > SND_SUN_AMD7930 depends on SND_SPARC && SBUS > SND_SPARC depends on SPARC > SPARC selects OF > > Hence, SND_SUN_AMD7930 depends on OF. Exactly my point. Read back what I wrote. TL;DR: It's *fine* to have _indirect_ dependency like this. -- With Best Regards, Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko wrote: > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven > wrote: > > > In any case it's not true. We have the platform drivers w/o legacy > > > users that are not dependent on OF. > > > > Example? ;-) > > i2c-owl.c In case you want more sound/sparc/amd7930.c And I believe I can find zillions of them. -- With Best Regards, Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven wrote: > On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko > wrote: > > On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven > > wrote: > > > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko > > > wrote: > > > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven > > > > wrote: > > > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > > > > wrote: > > > > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > > > > wrote: > > > > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven > > > > > > > > wrote: > > > > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang > > > > > > > > > wrote: > > > > ... > > > > > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > > > > > > > But why? > > > > > > > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > > > > > > > I'm not using it at all. Ask the author :-) > > > > > > > > > > > > But if we follow your logic, then we need to mark all the > > > > > > _platform_ drivers > > > > > > for x86 world as ACPI dependent? This sounds ugly. > > > > > > > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > > > > (2) no other way of instantiating their devices? > > > > > The first driver from the top of my memory I looked at is rtc-cmos: > > > > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > > > > from arch/x86/kernel/rtc.c. > > > > > > > > > > For drivers with only an .of_match_table(), and no legacy users > > > > > instantiating platform devices, we do have dependencies on OF. > > > > > > > > This is not true. Entire IIO subsystem is an example. > > > > > > Do you care to elaborate? > > > Three quarters of the IIO drivers are I2C and SPI drivers, and thus not > > > subject to the above. > > > > It seems I missed that you are talking about platform device drivers. > > OK. > > > In any case it's not true. We have the platform drivers w/o legacy > > users that are not dependent on OF. > > Example? ;-) i2c-owl.c > > They may _indirectly_ be dependent, but this is fine as I stated above > > when suggested to move ACPI dependency on ARCH_xxx level. > > As per the response from the driver maintainer > https://lore.kernel.org/linux-arm-kernel/bd8db435-24e1-5ab3-6b35-1d4d8a292...@hisilicon.com/, > there is no dependency on ARCH_HISI, so moving the ACPI dependency > up won't help. So, an ACPI dependency is simply not applicable here as it's a compile dependency as well, which is not a limitation for this driver. Again, talk to Masahiro how to handle this, but I don't see any good justification to have ACPI (compile time) dependency here. So, again NAK! -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin wrote:
Thanks for an update, it's getting better! My comments below.
> Add initial support for Murata SCA3300 3-axis industrial
> accelerometer with digital SPI interface. This device also
> provides a temperature measurement.
First of all, you forgot Cc reviewer(s).
> Datasheet: https://www.murata.com/en-global/products/sensor/accel/sca3300
>
No blank line in the tag block.
> Signed-off-by: Tomas Melin
...
> +/*
> + * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
> + */
One line.
...
> +#define SCA3300_MASK_STATUSGENMASK(8, 0)
> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
This feels like an orphan. Shouldn't you move it closer to the group
of corresponding register / etc definition?
> +#define SCA3300_REG_MODE 0xd
> +#define SCA3300_REG_SELBANK0x1f
> +#define SCA3300_REG_STATUS 0x6
> +#define SCA3300_REG_WHOAMI 0x10
> +
> +#define SCA3300_VALUE_DEVICE_ID0x51
> +#define SCA3300_VALUE_RS_ERROR 0x3
> +#define SCA3300_VALUE_SW_RESET 0x20
As above it doesn't shed a light for the relationship between
registers and these fields (?). I.o.w the names w/o properly grouped
(and probably commented) are confusing.
...
> +/**
> + * struct sca3300_data - device data
> + * @spi: SPI device structure
> + * @lock: Data buffer lock
> + * @txbuf: Transmit buffer
> + * @rxbuf: Receive buffer
Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> + * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
> + */
> +struct sca3300_data {
> + struct spi_device *spi;
> + struct mutex lock;
> + u8 txbuf[4];
> + u8 rxbuf[4];
> + struct {
> + s16 channels[4];
> + s64 ts __aligned(sizeof(s64));
> + } scan;
> +};
...
> + struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};
Missed space.
...
> + sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
Seems you ignored my comment. What is this 0x0? What is the meaning of it?
Same for all the rest magic numbers in the code.
> + /*
> +* return status error is cleared after reading status register once,
> +* expect EINVAL here
/*
* Fix the style of all your multi-line comments.
* You may follow this example.
*/
> +*/
> + if (ret != -EINVAL) {
> + dev_err(_data->spi->dev,
> + "error reading device status: %d\n", ret);
> + return ret;
> + }
> +
> + dev_err(_data->spi->dev, "device status: 0x%lx\n",
> + (val & SCA3300_MASK_STATUS));
Too many parentheses.
> + return 0;
> +}
...
> +static irqreturn_t sca3300_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct sca3300_data *data = iio_priv(indio_dev);
> + int bit, ret, val, i = 0;
> +
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> +indio_dev->masklength) {
> + ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> + );
> + if (ret) {
> + dev_err(>spi->dev,
> + "failed to read register, error: %d\n", ret);
> + goto out;
Does it mean interrupt is handled in this case?
Perhaps a comment why it's okay to consider so?
> + }
> + data->scan.channels[i++] = val;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, >scan,
> + iio_get_time_ns(indio_dev));
> +out:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
...
> + /*
> +* wait 1ms after SW-reset command
> +* wait 15ms for settling of signal paths
> +*/
> + usleep_range(16e3, 50e3);
+ blank line
> + ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, );
> + if (ret)
> + return ret;
> + ret = devm_iio_device_register(>dev, indio_dev);
> + if (ret) {
> + dev_err(>dev, "iio device register failed, error: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return ret;
Deduplicate it.
Simply leave the latter one.
> +}
...
> +
No need for this blank line.
> + .probe = sca3300_probe,
> +};
> +
Ditto.
> +module_spi_driver(sca3300_driver);
--
With Best Regards,
Andy Shevchenko
