| 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: <875xf0eho8.ffs@tglx>
Date: Wed, 06 Aug 2025 14:52:23 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Nick Hu <nick.hu@...ive.com>, anup@...infault.org, Alexandre Ghiti
<alex@...ti.fr>, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org
Cc: Nick Hu <nick.hu@...ive.com>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH v2 2/2] irqchip/riscv-aplic: Save and restore APLIC
registers
On Wed, Aug 06 2025 at 16:27, Nick Hu wrote:
> The APLIC may be powered down when the CPUs enter a deep sleep state.
> Therefore adding the APLIC save and restore functions to save and
> restore the states of APLIC.
Same comments vs. subject and change log.
>
> +void aplic_direct_restore(struct aplic_priv *priv)
> +{
> + struct aplic_direct *direct =
> + container_of(priv, struct aplic_direct, priv);
No line break required.
> + int cpu;
> +
> + for_each_cpu(cpu, &direct->lmask)
> + aplic_idc_set_delivery(per_cpu_ptr(&aplic_idcs, cpu), true);
> +static LIST_HEAD(aplics);
> +
> +static void aplic_restore(struct aplic_priv *priv)
> +{
> + int i;
> + u32 clrip;
See the documentation I linked you to and look for the chapter about
variable declarations.
> + writel(priv->saved_domaincfg, priv->regs + APLIC_DOMAINCFG);
> +#ifdef CONFIG_RISCV_M_MODE
> + writel(priv->saved_msiaddr, priv->regs + APLIC_xMSICFGADDR);
> + writel(priv->saved_msiaddrh, priv->regs + APLIC_xMSICFGADDRH);
> +#endif
> + for (i = 1; i <= priv->nr_irqs; i++) {
> + writel(priv->saved_sourcecfg[i - 1],
> + priv->regs + APLIC_SOURCECFG_BASE +
> + (i - 1) * sizeof(u32));
> + writel(priv->saved_target[i - 1],
> + priv->regs + APLIC_TARGET_BASE +
> + (i - 1) * sizeof(u32));
Sorry, but this is incomprehensible garbage.
Why are you starting with i = 1 when you subtract 1 from i at every
usage site?
u32 __iomem *regs = priv->regs;
for (i = 0; i < priv->nr_irqs; i++, regs++) {
writel(priv->saved_sourcecfg[i], regs + APLIC_SOURCECFG_BASE);
writel(priv->saved_target[i], regs + APLIC_TARGET_BASE);
}
See?
> + }
> +
> + for (i = 0; i <= priv->nr_irqs; i += 32) {
Off by one. This needs to be i < priv->nr_irqs, no?
> + writel(-1U, priv->regs + APLIC_CLRIE_BASE +
> + (i / 32) * sizeof(u32));
> + writel(priv->saved_ie[i / 32],
> + priv->regs + APLIC_SETIE_BASE +
> + (i / 32) * sizeof(u32));
> + }
> +
> + if (priv->nr_idcs) {
> + aplic_direct_restore(priv);
> + } else {
> + /* Re-trigger the interrupts */
> + for (i = 0; i <= priv->nr_irqs; i += 32) {
Same here
> + clrip = readl(priv->regs + APLIC_CLRIP_BASE +
> + (i / 32) * sizeof(u32));
> + writel(clrip, priv->regs + APLIC_SETIP_BASE +
> + (i / 32) * sizeof(u32));
> + }
And this can properly be combined into:
u32 __iomem *regs = priv->regs;
for (i = 0; i < priv->nr_irqs; i += 32, regs++) {
writel(-1U, regs + APLIC_CLRIE_BASE);
writel(priv->saved_ie[i / 32], regs + APLIC_SETIE_BASE);
if (!priv->nr_idcs)
writel(readl(regs + APLOC_CLRIP_BASE), regs + APLIC_SETIP_BASE);
}
if (priv->nr_idcs)
aplic_direct_restore(priv);
No?
> +}
> +
> +static void aplic_save(struct aplic_priv *priv)
> +{
> + int i;
> +
> + for (i = 1; i <= priv->nr_irqs; i++) {
> + priv->saved_target[i - 1] = readl(priv->regs +
> + APLIC_TARGET_BASE +
> + (i - 1) * sizeof(u32));
> + }
Oh well...
> +
> + for (i = 0; i <= priv->nr_irqs; i += 32) {
Off by one again ...
> + priv->saved_ie[i / 32] = readl(priv->regs +
> + APLIC_SETIE_BASE +
> + (i / 32) * sizeof(u32));
> + }
> +}
> +
> +static int aplic_syscore_suspend(void)
> +{
> + struct aplic_priv *priv;
> +
> + list_for_each_entry(priv, &aplics, head) {
> + aplic_save(priv);
> + }
Superflous brackets
> +
> + return 0;
> +}
> +
> +static void aplic_syscore_resume(void)
> +{
> + struct aplic_priv *priv;
> +
> + list_for_each_entry(priv, &aplics, head) {
> + aplic_restore(priv);
> + }
Ditto
> +}
> +
> +static struct syscore_ops aplic_syscore_ops = {
> + .suspend = aplic_syscore_suspend,
> + .resume = aplic_syscore_resume,
See documentation about struct definitions and initializers
> +};
> +
> +static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)
> +{
> + struct aplic_priv *priv = container_of(nb, struct aplic_priv, genpd_nb);
> +
> + switch (action) {
> + case GENPD_NOTIFY_PRE_OFF:
> + aplic_save(priv);
> + break;
> + case GENPD_NOTIFY_ON:
> + aplic_restore(priv);
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static void aplic_remove(void *data)
> +{
> + struct aplic_priv *priv = data;
> +
> + list_del(&priv->head);
How is this list operation serialized?
> + dev_pm_genpd_remove_notifier(priv->dev);
> +}
> +
> +static int aplic_add(struct device *dev, struct aplic_priv *priv)
> +{
> + int ret;
> +
> + list_add(&priv->head, &aplics);
> + /* Add genpd notifier */
> + priv->genpd_nb.notifier_call = aplic_pm_notifier;
> + ret = dev_pm_genpd_add_notifier(dev, &priv->genpd_nb);
> + if (ret && ret != -ENODEV && ret != -EOPNOTSUPP) {
What is this magic about? Lacks explanation and rationale.
> + list_del(&priv->head);
> + return ret;
...
> + /* For power management */
> + priv->saved_target = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> + GFP_KERNEL);
> + if (!priv->saved_target)
> + return -ENOMEM;
> +
> + priv->saved_sourcecfg = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> + GFP_KERNEL);
> + if (!priv->saved_sourcecfg)
> + return -ENOMEM;
> +
> + priv->saved_ie = devm_kzalloc(dev,
> + DIV_ROUND_UP(priv->nr_irqs, 32) * sizeof(u32),
> + GFP_KERNEL);
> + if (!priv->saved_ie)
> + return -ENOMEM;
Seriously? You allocate all of this seperately? Ever heard about the
concept of data structures?
struct savedregs {
u32 target;
u32 source;
u32 ie;
};
priv->savedregs = devm_kcalloc(dev, priv->nr_irqs, sizeof(*priv->savedregs, GFP_KERNEL);
Yes, I know you don't need as many ie registers, but that's not a
problem at all. You just need to write/read to/from index 0, 32, 64 ...
And your restore muck boils down to a single loop:
u32 __iomem *regs = priv->regs;
for (i = 0; i < priv->nr_irqs; i++, regs++) {
writel(priv->savedregs->source[i], regs + APLIC_SOURCECFG_BASE);
writel(priv->savedregs->target[i], regs + APLIC_TARGET_BASE);
if (i % 32)
continue;
writel(-1U, regs + APLIC_CLRIE_BASE);
writel(priv->saved_ie[i], regs + APLIC_SETIE_BASE);
if (!priv->nr_idcs)
writel(readl(regs + APLOC_CLRIP_BASE), regs + APLIC_SETIP_BASE);
}
That's too comprehensible, right?
> struct aplic_priv {
> + struct list_head head;
Why needs this to be at the top?
There is no reason at all. Not that it matters much as this data
structure is a trainwreck vs. cacheline efficiency anyway.
> struct device *dev;
> u32 gsi_base;
> u32 nr_irqs;
> @@ -31,6 +32,15 @@ struct aplic_priv {
> u32 acpi_aplic_id;
> void __iomem *regs;
> struct aplic_msicfg msicfg;
> + struct notifier_block genpd_nb;
> + u32 *saved_target;
So you aligned @head and @genpd_nb, but then you use random formatting
to turn this into visual clutter.
I'm amazed that this lot has received three Reviewed-by tags and nobody
found even the slightest issue with it. Oh well.
Thanks,
tglx
Powered by blists - more mailing lists