linux-kernel - Re: [PATCH v12 5/6] dmaengine: pl330: Add PM sleep support

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]

Message-id: <1415972831.25433.3.camel@AMDC1943>
Date:	Fri, 14 Nov 2014 14:47:11 +0100
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Ulf Hansson <ulf.hansson@...aro.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	Russell King <linux@....linux.org.uk>,
	Dan Williams <dan.j.williams@...el.com>,
	Vinod Koul <vinod.koul@...el.com>,
	Alan Stern <stern@...land.harvard.edu>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	dmaengine@...r.kernel.org, Kevin Hilman <khilman@...nel.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH v12 5/6] dmaengine: pl330: Add PM sleep support

On pią, 2014-11-14 at 14:31 +0100, Ulf Hansson wrote:
> On 14 November 2014 09:50, Krzysztof Kozlowski <k.kozlowski@...sung.com> wrote:
> > Add system suspend/resume capabilities to the pl330 driver so the amba
> > bus clock could be also unprepared to conserve energy.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
> > ---
> >  drivers/dma/pl330.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index c3bd3584f261..e499bb118f0a 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -2627,6 +2627,46 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> >         return 0;
> >  }
> >
> > +/*
> > + * Runtime PM callbacks are provided by amba/bus.c driver.
> > + *
> > + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
> > + * bus driver will only disable/enable the clock in runtime PM callbacks.
> > + */
> > +static int __maybe_unused pl330_suspend(struct device *dev)
> > +{
> > +       struct amba_device *pcdev = to_amba_device(dev);
> > +
> > +       pm_runtime_disable(dev);
> > +
> > +       if (!pm_runtime_status_suspended(dev)) {
> > +               /* amba did not disable the clock */
> > +               amba_pclk_disable(pcdev);
> > +       }
> > +       amba_pclk_unprepare(pcdev);
> 
> I would also invoke pm_runtime_set_suspended() here, to reflect that's
> the current runtime PM state of the device.

Strictly speaking the device is not runtime suspended in that moment. PM
runtime callbacks were not called. Although the device status looks like
runtime suspended (clocks disabled) but I think the whole goal here was
to avoid touching runtime PM because this is system sleep.


> I guess I sounds like broken record :-), but using
> pm_runtime_force_suspend() would help to prevent some code duplication
> here.
> 
> Something like this:
> 
> pm_runtime_force_suspend()
> if (pm_runtime_is_irq_safe())
>    amba_pclk_unprepare(pcdev);

With !CONFIG_PM_RUNTIME this would leave clocks prepared...

Thanks for feedback!
Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/