linux-kernel - Re: [PATCH 5/7] irqchip/renesas-rzg2l: cache registers on suspend/resume

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Oct 2023 19:57:23 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Claudiu <claudiu.beznea@...on.dev>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        geert+renesas@...der.be, magnus.damm@...il.com,
        mturquette@...libre.com, sboyd@...nel.org,
        prabhakar.mahadev-lad.rj@...renesas.com
Cc:     linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
        Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 5/7] irqchip/renesas-rzg2l: cache registers on
 suspend/resume

On Mon, Oct 23 2023 at 13:22, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> Cache registers content when going to suspend and restore them in resume
> as these may be lost when switching to deep sleep states. With this
> driver data has been marked as static to be able to play with it
> in struct syscon_ops::{suspend, resume}.

I have no idea what you are trying to tell me here. Why do the
suspend/resume callbacks need a static data structure and cannot operate
on a pointer which wastes less builtin memory when the driver is not
used?

Also "play with it" is definitely not a technical term. See
Documentation/process/* which has lots of explanations how to write
proper change logs.

> Because IA55 input is from pin controller and IA55 resumes before pin
> controller we don't restore interrupt enable bits here but let the
> pinctrl to do it on IA55 behalf after pins are in proper state to
> avoid invalid interrupts.
> +
> +static struct rzg2l_irqc_priv {
>  	void __iomem *base;
>  	struct irq_fwspec fwspec[IRQC_NUM_IRQ];
>  	raw_spinlock_t lock;
> -};
> +	struct rzg2l_irqc_reg_cache cache;
> +} priv;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

>  
>  static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data)
>  {
> @@ -238,6 +251,37 @@ static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
>  	return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
>  }
>  
> +static int rzg2l_irqc_irq_suspend(void)
> +{
> +	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
> +
> +	cache->iitsr = readl_relaxed(priv.base + IITSR);
> +	for (u8 i = 0; i < 2; i++)
> +		cache->titsr[i] = readl_relaxed(priv.base + TITSR(i));
> +
> +	return 0;
> +}
> +
> +static void rzg2l_irqc_irq_resume(void)
> +{
> +	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
> +	u8 i;
> +
> +	/*
> +	 * Restore only interrupt type. TSSRx will be restored at the
> +	 * request of pin controller to avoid spurious interrupts due
> +	 * to invalid PIN states.
> +	 */
> +	for (i = 0; i < 2; i++)
> +		writel_relaxed(cache->titsr[i], priv.base + TITSR(i));
> +	writel_relaxed(cache->iitsr, priv.base + IITSR);
> +}
> +
> +static struct syscore_ops rzg2l_irqc_syscore_ops = {
> +	.suspend = rzg2l_irqc_irq_suspend,
> +	.resume = rzg2l_irqc_irq_resume,
> +};

Ditto.

>  static const struct irq_chip irqc_chip = {
>  	.name			= "rzg2l-irqc",
>  	.irq_eoi		= rzg2l_irqc_eoi,
> @@ -323,7 +367,6 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
>  	struct irq_domain *irq_domain, *parent_domain;
>  	struct platform_device *pdev;
>  	struct reset_control *resetn;
> -	struct rzg2l_irqc_priv *priv;

Make this pointer static at the top level and leave the rest of the code
alone and please give it a proper name. "priv" at the file level is
really non-descriptive.

Thanks,

        tglx