Re: [PATCH v6 04/21] RISC-V: Define indirect CSR access helpers

[Atish Patra <atish.patra@linux.dev>]

On 6/8/26 11:15 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Unsafe statement-expression macro definitions causing delayed evaluation and variable shadowing hazards.
> --
>
> commit 572b7555547c1b5bc07701482ba67d379f6a2177
> Author: Atish Patra <atishp@rivosinc.com>
>
> RISC-V: Define indirect CSR access helpers
>
> This commit introduces helper macros to read, write, and perform WARL
> operations on indirect CSRs. The macros use statement-expressions to handle
> the multiple instructions required while disabling interrupts.
>
>> diff --git a/arch/riscv/include/asm/csr_ind.h b/arch/riscv/include/asm/csr_ind.h
>> new file mode 100644
>> index 0000000000000..6fd7d44dc640a
>> --- /dev/null
>> +++ b/arch/riscv/include/asm/csr_ind.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024 Rivos Inc.
>> + */
>> +
>> +#ifndef _ASM_RISCV_CSR_IND_H
>> +#define _ASM_RISCV_CSR_IND_H
>> +
>> +#include <linux/irqflags.h>
>> +
>> +#include <asm/csr.h>
>> +
>> +#define csr_ind_read(iregcsr, iselbase, iseloff) ({		\
>> +	unsigned long __value = 0;				\
>> +	unsigned long __flags;					\
> [Severity: Medium]
> Will these generic variable names (__value, __flags) cause shadowing issues if
> the caller passes in arguments with the same names?
>
> Evaluating macro arguments into unique local variables before entering the
> IRQ-disabled region would avoid this regression.
>
>> +	local_irq_save(__flags);				\
>> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
>> +	__value = csr_read(iregcsr);				\
>> +	local_irq_restore(__flags);				\
>> +	__value;						\
>> +})
>> +
>> +#define csr_ind_write(iregcsr, iselbase, iseloff, value) ({	\
>> +	unsigned long __flags;					\
>> +	local_irq_save(__flags);				\
>> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
>> +	csr_write(iregcsr, (value));				\
> [Severity: Medium]
> Since value is evaluated here after CSR_ISELECT has been programmed, what
> happens if a caller passes an expression that accesses another indirect CSR,
> such as a nested csr_ind_read(), as the value argument?
>
> It appears the nested call would overwrite CSR_ISELECT, causing the write to
> silently corrupt the wrong hardware register. Evaluating arguments into local
> variables before disabling interrupts would prevent this, and also avoid the
> risk of evaluating expressions with side effects while interrupts are disabled.
>
>> +	local_irq_restore(__flags);				\
>> +})
>> +
>> +#define csr_ind_warl(iregcsr, iselbase, iseloff, warl_val) ({	\
>> +	unsigned long __old_val = 0, __value = 0;		\
>> +	unsigned long __flags;					\
> [Severity: Medium]
> Do these generic local variables risk shadowing caller arguments?
>
>> +	local_irq_save(__flags);				\
>> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
>> +	__old_val = csr_read(iregcsr);				\
>> +	csr_write(iregcsr, (warl_val));				\
> [Severity: Medium]
> Is there a risk that evaluating warl_val here could overwrite CSR_ISELECT if
> the caller provides an expression that performs another indirect CSR access?
>
>> +	__value = csr_read(iregcsr);				\
>> +	csr_write(iregcsr, __old_val);				\
>> +	local_irq_restore(__flags);				\
>> +	__value;						\
>> +})
>> +
>> +#endif

Given the callers, I don't think the possibilities raised by Sashiko can 
actually happen.
It's a macro. So the new callers should ensure they are invoking it with 
proper argument.

N/A.