linux-kernel - RE: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM

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] [day] [month] [year] [list]

Message-ID: <DB8PR04MB67950EC575602273EDF35DD2E6720@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Mon, 27 Jul 2020 02:44:55 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Marc Zyngier <maz@...nel.org>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "jason@...edaemon.net" <jason@...edaemon.net>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM


> -----Original Message-----
> From: Marc Zyngier <maz@...nel.org>
> Sent: 2020年7月25日 21:05
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: tglx@...utronix.de; jason@...edaemon.net; shawnguo@...nel.org;
> s.hauer@...gutronix.de; kernel@...gutronix.de; festevam@...il.com;
> dl-linux-imx <linux-imx@....com>; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM
> 
> On Mon, 20 Jul 2020 11:42:37 +0100,
> Joakim Zhang <qiangqing.zhang@....com> wrote:
> >
> > When system suspended, we could explicitly disable clock to save power.
> > And we need save registers' state since it could be lost after power
> > off.
> >
> > Implement PM which will:
> > 1) Without CONFIG_PM, clock is always on after probe stage.
> > 2) With CONFIG_PM, clock is off after probe stage.
> > 3) Disable clock and save registers' state when do system suspend and
> > enable clock and restore registers' state while system resume.
> > 4) Make Power Domain framework be able to shutdown the corresponding
> > power domain of this device.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> > ---
> >  drivers/irqchip/irq-imx-intmux.c | 70
> > +++++++++++++++++++++++++++++++-
> >  1 file changed, 68 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-intmux.c
> > b/drivers/irqchip/irq-imx-intmux.c
> > index c27577c81126..5971603cc607 100644
> > --- a/drivers/irqchip/irq-imx-intmux.c
> > +++ b/drivers/irqchip/irq-imx-intmux.c
> > @@ -53,6 +53,7 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #define CHANIER(n)	(0x10 + (0x40 * n))
> >  #define CHANIPR(n)	(0x20 + (0x40 * n))
> > @@ -60,6 +61,7 @@
> >  #define CHAN_MAX_NUM		0x8
> >
> >  struct intmux_irqchip_data {
> > +	struct irq_chip		chip;
> >  	int			chanidx;
> >  	int			irq;
> >  	struct irq_domain	*domain;
> > @@ -70,6 +72,7 @@ struct intmux_data {
> >  	void __iomem			*regs;
> >  	struct clk			*ipg_clk;
> >  	int				channum;
> > +	u32				*saved_reg;
> >  	struct intmux_irqchip_data	irqchip_data[];
> >  };
> >
> > @@ -120,8 +123,10 @@ static struct irq_chip imx_intmux_irq_chip = {
> > static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
> >  			      irq_hw_number_t hwirq)
> >  {
> > -	irq_set_chip_data(irq, h->host_data);
> > -	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, handle_level_irq);
> > +	struct intmux_irqchip_data *data = h->host_data;
> > +
> > +	irq_set_chip_data(irq, data);
> > +	irq_set_chip_and_handler(irq, &data->chip, handle_level_irq);
> >
> >  	return 0;
> >  }
> > @@ -232,6 +237,19 @@ static int imx_intmux_probe(struct platform_device
> *pdev)
> >  	data->channum = channum;
> >  	raw_spin_lock_init(&data->lock);
> >
> > +	if (IS_ENABLED(CONFIG_PM)) {
> > +		/* save CHANIER register */
> > +		data->saved_reg = devm_kzalloc(&pdev->dev,
> > +					       sizeof(unsigned int) * channum,
> 
> This isn't consistent with the type of data->saved_reg. Consider using
> sizeof(*data->saved_reg), which is guaranteed to be the right type.

Sorry for my mistake, I forgot to modify this unit.

> It also begs the question: since this saved_reg array is allocated on a per
> channel basis, why don't you have a per-channel additional u32 in the
> intmux_irqchip_data structure instead? This would sidestep this extra
> allocation altogether.

Yes, this make sense. Thanks.

Best Regards,
Joakim Zhang
> Thanks,
> 
> 	M.
> 
> --
> Without deviation from the norm, progress is not possible.