Re: [PATCH v2 2/2] RISC-V: Add common csr_read_num() and csr_write_num() functions

[Yao Zi <ziyao@disroot.org>]

On Tue, Aug 19, 2025 at 09:00:03AM +0530, Anup Patel wrote:
> On Tue, Aug 19, 2025 at 8:56 AM Yao Zi <ziyao@disroot.org> wrote:
> >
> > On Mon, Aug 18, 2025 at 08:06:00PM +0530, Anup Patel wrote:
> > > In RISC-V, there is no CSR read/write instruction which takes CSR
> > > number via register so add common csr_read_num() and csr_write_num()
> > > functions which allow accessing certain CSRs by passing CSR number
> > > as parameter. These common functions will be first used by the
> > > ACPI CPPC driver and RISC-V PMU driver.
> > >
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/csr.h |   3 +
> > >  arch/riscv/kernel/Makefile   |   1 +
> > >  arch/riscv/kernel/csr.c      | 165 +++++++++++++++++++++++++++++++++++
> > >  drivers/acpi/riscv/cppc.c    |  17 ++--
> > >  drivers/perf/riscv_pmu.c     |  54 ++----------
> > >  5 files changed, 184 insertions(+), 56 deletions(-)
> > >  create mode 100644 arch/riscv/kernel/csr.c
> > >
> > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> > > index 6fed42e37705..1540626b3540 100644
> > > --- a/arch/riscv/include/asm/csr.h
> > > +++ b/arch/riscv/include/asm/csr.h
> > > @@ -575,6 +575,9 @@
> > >                             : "memory");                      \
> > >  })
> > >
> > > +extern unsigned long csr_read_num(unsigned long csr_num, int *out_err);
> > > +extern void csr_write_num(unsigned long csr_num, unsigned long val, int *out_err);
> >
> > I think it's more consistent to directly return the error code, and for
> > csr_read_num, we could pass out the read value by a pointer. e.g.
> >
> >         int csr_read_num(unsigned long csr_num, unsigned long *val);
> >         int csr_write_num(unsigned long csr_num, unsigned long val);
> >
> > This allows the caller to eliminate a variable for temporarily storing
> > the error code if they use it just after the invokation, and fits the
> > common convention of Linux better.
> 
> Drew had similar comments so see my response in the previous
> patch revision. (Refer, https://www.spinics.net/lists/kernel/msg5808113.html)

Thanks for the reference.

> I had considered this but the problem with this approach is that
> individual switch cases in csr_read_num() become roughly 4
> instructions because value read from CSR has to written to a memory
> location.

You could return a structure smaller than or equal to 2 * XLEN from
csr_read_num(), according to the ABI it could be passed in a0 and a1 and
thus should require no memory operation.

Let's assume we have

	struct __csr_read_ret {
		long error;
		unsigned long value;
	};

	struct __csr_read_ret __csr_read_num(unsigned long csr_num);

Then a wrapper like

	/* piece of untested code */
	static inline int csr_read_num(unsigned long csr_num,
				       unsigned long *val)
	{
		struct __csr_read_ret ret = __csr_read_num(csr_num);
		*val = ret.value;
		return ret.error;
	}

could provide an interface that I've talked about earlier, and it
follows the kernel's convention.

> The current approach results in just 2 instructions for each
> switch-case. Additionally, the current prototypes of csr_read_num()
> and csr_write_num() are closer to csr_read() and csr_write()
> respectively.

But csr_read() and csr_write() never directly raise errors that is
expected to be handled by the caller. I don't think it's a fair
comparison.

> Regards,
> Anup

Best regards,
Yao Zi