| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPqJEFr1FfuHYX=BP3BXAsZ0V4JFDqzu7_jjPVgcijOFQzddmQ@mail.gmail.com>
Date: Fri, 23 Jun 2023 17:49:22 +0800
From: Eric Lin <eric.lin@...ive.com>
To: Conor Dooley <conor@...nel.org>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
palmer@...belt.com, paul.walmsley@...ive.com,
aou@...s.berkeley.edu, maz@...nel.org, chenhuacai@...nel.org,
baolu.lu@...ux.intel.com, will@...nel.org,
kan.liang@...ux.intel.com, nnac123@...ux.ibm.com,
pierre.gondois@....com, huangguangbin2@...wei.com, jgross@...e.com,
chao.gao@...el.com, maobibo@...ngson.cn,
linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, dslin1010@...il.com,
Nick Hu <nick.hu@...ive.com>, Zong Li <zong.li@...ive.com>
Subject: Re: [PATCH 1/3] soc: sifive: Add SiFive private L2 cache support
Hi Conor,
On Sat, Jun 17, 2023 at 5:05 AM Conor Dooley <conor@...nel.org> wrote:
>
> Hey Eric,
>
> On Fri, Jun 16, 2023 at 02:32:08PM +0800, Eric Lin wrote:
> > This adds SiFive private L2 cache driver which will show
> > cache config information when booting and add cpu hotplug
> > callback functions.
> >
> > Signed-off-by: Eric Lin <eric.lin@...ive.com>
> > Signed-off-by: Nick Hu <nick.hu@...ive.com>
>
> Missing a Co-developed-by for Nick?
Yes, I'll add Co-developed-by for Nick in v2. Thanks.
>
>
> > +static void pl2_config_read(void __iomem *pl2_base, int cpu)
> > +{
> > + u32 regval, bank, way, set, cacheline;
> > +
> > + regval = readl(pl2_base);
> > + bank = regval & 0xff;
> > + pr_info("in the CPU: %d\n", cpu);
> > + pr_info("No. of Banks in the cache: %d\n", bank);
> > + way = (regval & 0xff00) >> 8;
> > + pr_info("No. of ways per bank: %d\n", way);
> > + set = (regval & 0xff0000) >> 16;
> > + pr_info("Total sets: %llu\n", (uint64_t)1 << set);
> > + cacheline = (regval & 0xff000000) >> 24;
> > + pr_info("Bytes per cache block: %llu\n", (uint64_t)1 << cacheline);
> > + pr_info("Size: %d\n", way << (set + cacheline));
> > +}
>
> Isn't this basically all information that we get anyway in sysfs based
> on what gets put into the DT, except printed out once per CPU at
> boottime?
> If there's reason to keep it, please do as suggested by Ben and cut down
> the number of lines emitted. Look at the ccache one for comparison:
> static void ccache_config_read(void)
> {
> u32 cfg;
>
> cfg = readl(ccache_base + SIFIVE_CCACHE_CONFIG);
> pr_info("%llu banks, %llu ways, sets/bank=%llu, bytes/block=%llu\n",
> FIELD_GET(SIFIVE_CCACHE_CONFIG_BANK_MASK, cfg),
> FIELD_GET(SIFIVE_CCACHE_CONFIG_WAYS_MASK, cfg),
> BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_SETS_MASK, cfg)),
> BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_BLKS_MASK, cfg)));
>
> cfg = readl(ccache_base + SIFIVE_CCACHE_WAYENABLE);
> pr_info("Index of the largest way enabled: %u\n", cfg);
> }
> It'd also be good to print the same things as the ccache, no?
>
Yes, I'll cut down the number of lines as the ccache in v2. Thanks for
the suggestion.
> > +static int sifive_pl2_cache_dev_probe(struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > + int cpu, ret = -EINVAL;
> > + struct device_node *cpu_node, *pl2_node;
> > + struct sifive_pl2_state *pl2_state = NULL;
> > + void __iomem *pl2_base;
>
> Please pick a sensible ordering for variables. IDC if it is reverse xmas
> tree, or sorting by types, but this just seems quite random..
>
Yes, I'll sort by type in v2.
> > + /* Traverse all cpu nodes to find the one mapping to its pl2 node. */
> > + for_each_cpu(cpu, cpu_possible_mask) {
> > + cpu_node = of_cpu_device_node_get(cpu);
> > + pl2_node = of_parse_phandle(cpu_node, "next-level-cache", 0);
> > +
> > + /* Found it! */
> > + if (dev_of_node(&pdev->dev) == pl2_node) {
> > + /* Use cpu to get its percpu data sifive_pl2_state. */
> > + pl2_state = per_cpu_ptr(&sifive_pl2_state, cpu);
> > + break;
> > + }
> > + }
> > +
> > + if (!pl2_state) {
> > + pr_err("Not found the corresponding cpu_node in dts.\n");
>
> I don't think this error message is going to be helpful in figuring out
> where the problem is on a machine with many of the caches. More
> information about *which* cache caused it would be good.
> Also it is not grammatically correct, it should read something like
> "Failed to find CPU node for cache@abc" or something along those lines.
>
OK, I'll rewrite the error message to make it more helpful for the user.
I'll fix it in v2. Thanks for the suggestion.
> > + goto early_err;
>
> early_err just returns ret. Why not just return the error directly?
>
Yeah, it can just return ret. I'll fix it in v2.
> > + }
> > +
> > + /* Set base address of select and counter registers. */
> > + pl2_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > + if (IS_ERR(pl2_base)) {
> > + ret = PTR_ERR(pl2_base);
> > + goto early_err;
> > + }
> > +
> > + /* Print pL2 configs. */
> > + pl2_config_read(pl2_base, cpu);
> > + pl2_state->pl2_base = pl2_base;
> > +
> > + return 0;
> > +
> > +early_err:
> > + return ret;
> > +}
>
> > +static struct platform_driver sifive_pl2_cache_driver = {
> > + .driver = {
> > + .name = "SiFive-pL2-cache",
> > + .of_match_table = sifive_pl2_cache_of_ids,
> > + },
> > + .probe = sifive_pl2_cache_dev_probe,
> > +};
> > +
> > +static int __init sifive_pl2_cache_init(void)
> > +{
> > + int ret;
> > +
> > + ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> > + "soc/sifive/pl2:online",
> > + sifive_pl2_online_cpu,
> > + sifive_pl2_offline_cpu);
>
> Got some weird use of whitespace here & above, please remove the spaces.
>
Yes, I'll remove the whitespace in v2.
Thanks for the review.
Best Regards,
Eric Lin.
> Cheers,
> Conor.
Powered by blists - more mailing lists