Re: [PATCH v6 2/3] riscv: introduce percpu.h into include/asm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Inconsistent naming and semantics for 8/16-bit vs 32/64-bit 'andnot' percpu operations.
--

commit 8c96e5a266e5953bcbbc15476c253fbdd307508b
Author: Yunhui Cui <cuiyunhui@bytedance.com>

riscv: introduce percpu.h into include/asm

Add RISC-V specific this_cpu helpers so common percpu operations can use
short architecture sequences instead of the generic implementation.
Native-width operations use AMOs, while 8/16-bit operations use Zabha when
available and a local 32-bit LR/SC fallback otherwise.

> diff --git a/arch/riscv/include/asm/percpu.h b/arch/riscv/include/asm/percpu.h
> new file mode 100644
> index 0000000000000..34483253e946f
> --- /dev/null
> +++ b/arch/riscv/include/asm/percpu.h

[ ... ]

> +#ifdef CONFIG_64BIT
> +#define PERCPU_OP(name, amo_insn)					\
> +	__PERCPU_AMO_OP_CASE(.w, name, 32, amo_insn)			\
> +	__PERCPU_AMO_OP_CASE(.d, name, 64, amo_insn)
> +#else
> +#define PERCPU_OP(name, amo_insn)					\
> +	__PERCPU_AMO_OP_CASE(.w, name, 32, amo_insn)
> +#endif
> +
> +PERCPU_OP(add, add)
> +PERCPU_OP(andnot, and)

[Severity: Low]
This isn't a bug, but is the naming here intentional? This defines
__percpu_andnot_amo_case_32 and __percpu_andnot_amo_case_64, but since it
passes the 'and' instruction and operates on the value directly, it performs
a standard bitwise and rather than an and-not.

> +PERCPU_OP(or, or)

[ ... ]

> +#define PERCPU_OP_8_16(op_name, op, expr, final_op)			\
> +	PERCPU_8_16_OP(op_name, op, 8, .b, u8, expr, final_op);		\
> +	PERCPU_8_16_OP(op_name, op, 16, .h, u16, expr, final_op)
> +
> +PERCPU_OP_8_16(add, add, val, add)
> +PERCPU_OP_8_16(andnot, and, ~(val), and)

[Severity: Low]
Conversely, for 8-bit and 16-bit operations, the andnot operation actually
performs an and-not by passing ~(val).

> +PERCPU_OP_8_16(or, or, val, or)

[ ... ]

> +#define this_cpu_and_1(pcp, val)	_pcp_protect(__percpu_andnot_amo_case_8, pcp, ~(val))
> +#define this_cpu_and_2(pcp, val)	_pcp_protect(__percpu_andnot_amo_case_16, pcp, ~(val))
> +#define this_cpu_and_4(pcp, val)	_pcp_protect(__percpu_andnot_amo_case_32, pcp, val)
> +
> +#ifdef CONFIG_64BIT
> +#define this_cpu_and_8(pcp, val)	_pcp_protect(__percpu_andnot_amo_case_64, pcp, val)
> +#endif

[Severity: Low]
Because of the semantic difference between the underlying helpers, the 1 and 2
byte wrappers pass ~(val) while the 4 and 8 byte wrappers pass val.

Would it be clearer to harmonize the semantics across all sizes so the
wrappers do not have to compensate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701122948.57584-1-cuiyunhui@bytedance.com?part=2