Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230710-equipment-stained-dd042d66ba5d@wendy> <20230710-banker-visible-4c4cb3685dc1@wendy>
In-Reply-To: <20230710-banker-visible-4c4cb3685dc1@wendy>
From:   Evan Green <evan@rivosinc.com>
Date:   Wed, 12 Jul 2023 10:34:06 -0700
Message-ID: <CALs-HsstyS+qFJZjb7Lv_2LmygfhA_EAeEdoCSRhtaXnYfhw0w@mail.gmail.com>
Subject: Re: [PATCH v4 05/11] RISC-V: repurpose riscv_isa_ext array in riscv_fill_hwcap()
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     palmer@dabbelt.com, conor@kernel.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Jonathan Corbet <corbet@lwn.net>,
        Andrew Jones <ajones@ventanamicro.com>,
        Heiko Stuebner <heiko.stuebner@vrull.eu>,
        Sunil V L <sunilvl@ventanamicro.com>,
        linux-doc@vger.kernel.org, linux-riscv@lists.infradead.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 10, 2023 at 2:36=E2=80=AFAM Conor Dooley <conor.dooley@microchi=
p.com> wrote:
>
> In riscv_fill_hwcap() riscv_isa_ext array can be looped over, rather
> than duplicating the list of extensions with individual
> SET_ISA_EXT_MAP() usage. While at it, drop the statement-of-the-obvious
> comments from the struct, rename uprop to something more suitable for
> its new use & constify the members.
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Changes in v2:
> - Delete the now unused definition
> ---
>  arch/riscv/include/asm/hwcap.h |  7 ++-----
>  arch/riscv/kernel/cpu.c        |  5 +++--
>  arch/riscv/kernel/cpufeature.c | 26 +++++++-------------------
>  3 files changed, 12 insertions(+), 26 deletions(-)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwca=
p.h
> index 7a57e6109aef..2460ac2fc7ed 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -55,7 +55,6 @@
>  #define RISCV_ISA_EXT_ZIHPM            42
>
>  #define RISCV_ISA_EXT_MAX              64
> -#define RISCV_ISA_EXT_NAME_LEN_MAX     32
>
>  #ifdef CONFIG_RISCV_M_MODE
>  #define RISCV_ISA_EXT_SxAIA            RISCV_ISA_EXT_SMAIA
> @@ -70,10 +69,8 @@
>  unsigned long riscv_get_elf_hwcap(void);
>
>  struct riscv_isa_ext_data {
> -       /* Name of the extension displayed to userspace via /proc/cpuinfo=
 */
> -       char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
> -       /* The logical ISA extension ID */
> -       unsigned int isa_ext_id;
> +       const unsigned int id;
> +       const char *name;
>  };
>
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index bf93293d51f3..aa17eeb0ec9a 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -168,9 +168,10 @@ static void print_isa_ext(struct seq_file *f)
>  {
>         for (int i =3D 0; i < riscv_isa_ext_count; i++) {
>                 const struct riscv_isa_ext_data *edata =3D &riscv_isa_ext=
[i];
> -               if (!__riscv_isa_extension_available(NULL, edata->isa_ext=
_id))
> +               if (!__riscv_isa_extension_available(NULL, edata->id))
>                         continue;
> -               seq_printf(f, "_%s", edata->uprop);
> +
> +               seq_printf(f, "_%s", edata->name);
>         }
>  }
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeatur=
e.c
> index fb476153fffc..6d8cd45af723 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -99,11 +99,10 @@ static bool riscv_isa_extension_check(int id)
>         return true;
>  }
>
> -#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
> -       {                                                       \
> -               .uprop =3D #UPROP,                                \
> -               .isa_ext_id =3D EXTID,                            \
> -       }
> +#define __RISCV_ISA_EXT_DATA(_name, _id) {     \
> +       .name =3D #_name,                         \
> +       .id =3D _id,                              \
> +}
>
>  /*
>   * The canonical order of ISA extension names in the ISA string is defin=
ed in
> @@ -366,20 +365,9 @@ void __init riscv_fill_hwcap(void)
>                                         set_bit(nr, isainfo->isa);
>                                 }
>                         } else {
> -                               /* sorted alphabetically */
> -                               SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SM=
AIA);
> -                               SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SS=
AIA);
> -                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT=
_SSCOFPMF);
> -                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SST=
C);
> -                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_=
SVINVAL);
> -                               SET_ISA_EXT_MAP("svnapot", RISCV_ISA_EXT_=
SVNAPOT);
> -                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_S=
VPBMT);
> -                               SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA)=
;
> -                               SET_ISA_EXT_MAP("zbb", RISCV_ISA_EXT_ZBB)=
;
> -                               SET_ISA_EXT_MAP("zbs", RISCV_ISA_EXT_ZBS)=
;
> -                               SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_Z=
ICBOM);
> -                               SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_Z=
ICBOZ);
> -                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_=
EXT_ZIHINTPAUSE);
> +                               for (int i =3D 0; i < riscv_isa_ext_count=
; i++)
> +                                       SET_ISA_EXT_MAP(riscv_isa_ext[i].=
name,

Does this still work, given that SET_ISA_EXT_MAP() does sizeof(name)
to get the string length? I worry that line of the macro is now
evaluating to a constant sizeof(pointer), and the macro needs to
change to use strlen().

-Evan