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

[Paul Walmsley <pjw@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

On Thu, 30 Oct 2025, Anup Patel wrote:

> On Thu, Oct 30, 2025 at 12:35 AM Paul Walmsley <pjw@kernel.org> wrote:
> > On Tue, 14 Oct 2025, 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.
> >
> > This patch also (silently) removes the CSR number filtering, e.g.
> >
> > > diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> > > index 42c1a9052470..fe491937ed25 100644
> > > --- a/drivers/acpi/riscv/cppc.c
> > > +++ b/drivers/acpi/riscv/cppc.c
> > > @@ -65,24 +65,19 @@ static void sbi_cppc_write(void *write_data)
> > >  static void cppc_ffh_csr_read(void *read_data)
> > >  {
> > >       struct sbi_cppc_data *data = (struct sbi_cppc_data *)read_data;
> > > +     int err;
> > >
> > > -     switch (data->reg) {
> > > -     /* Support only TIME CSR for now */
> > > -     case CSR_TIME:
> > > -             data->ret.value = csr_read(CSR_TIME);
> > > -             data->ret.error = 0;
> > > -             break;
> > > -     default:
> > > -             data->ret.error = -EINVAL;
> > > -             break;
> > > -     }
> >
> > ... the above code, and:
> 
> The switch case is incomplete for cppc_ffh_csr_read().
> Also, csr_read_num() already does appropriate filtering
> so the switch case over here is now redundant.
> 
> >
> > >  /*
> > >   * Read the CSR of a corresponding counter.
> > >   */
> > >  unsigned long riscv_pmu_ctr_read_csr(unsigned long csr)
> > >  {
> > > -     if (csr < CSR_CYCLE || csr > CSR_HPMCOUNTER31H ||
> > > -        (csr > CSR_HPMCOUNTER31 && csr < CSR_CYCLEH)) {
> > > -             pr_err("Invalid performance counter csr %lx\n", csr);
> > > -             return -EINVAL;
> >
> > ... the above code.
> >
> > I'm thinking that we probably want to keep the CSR number filtering code
> > in; at least, I can't think of a good reason to remove it.  Care to add it
> > back in?
> 
> We can potentially have custom CSRs as hardware counters
> hence the CSR filtering over here is already incomplete. Plus,
> csr_read_num() already does the CSR filtering and returns
> failure for inappropriate CSR number.

OK.  To me, the most notable changes in this patch are the changes in the 
filters for both reads and writes for the two different call sites, ACPI 
FFH and PMU.  It would be good to document these filter changes directly 
in the patch description, along with the rationale.

I'm also not yet completely convinced that we should allow both of these 
call sites to read and write any custom CSR.  But maybe the updated patch 
description might be convincing...


- Paul

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv