linux-kernel - Re: [PATCH v8 3/3] dmaengine: pl330: Don't require irq-safe runtime 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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 10 Feb 2017 10:20:04 +0530
From:   Vinod Koul <vinod.koul@...el.com>
To:     Marek Szyprowski <m.szyprowski@...sung.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     linux-samsung-soc@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Arnd Bergmann <arnd@...db.de>, Inki Dae <inki.dae@...sung.com>
Subject: Re: [PATCH v8 3/3] dmaengine: pl330: Don't require irq-safe runtime
 PM

On Thu, Feb 09, 2017 at 03:22:51PM +0100, Marek Szyprowski wrote:

> +static int pl330_set_slave(struct dma_chan *chan, struct device *slave)
> +{
> +	struct dma_pl330_chan *pch = to_pchan(chan);
> +	struct pl330_dmac *pl330 = pch->dmac;
> +	int i;
> +
> +	mutex_lock(&pl330->rpm_lock);
> +
> +	for (i = 0; i < pl330->num_peripherals; i++) {
> +		if (pl330->peripherals[i].chan.slave == slave &&
> +		    pl330->peripherals[i].slave_link) {
> +			pch->slave_link = pl330->peripherals[i].slave_link;
> +			goto done;
> +		}
> +	}
> +
> +	pch->slave_link = device_link_add(slave, pl330->ddma.dev,
> +				       DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);

So you are going to add the link on channel allocation and tear down on the
freeup. I am not sure I really like the idea here.

First, these thing shouldn't be handled in the drivers. These things should
be set in core and each driver setting the links doesn't sound great to me.

Second, should the link be always there and we only mange the state? Here it
seems that we have link being created and destroyed, so why not mark it
ACTIVE and DORMANT instead...

Lastly, looking at th description of the issue here, am perceiving (maybe my
understanding is not quite right here) that you have an IP block in SoC
which has multiple things and share common stuff and doing right PM is a
challenge for you, right?

-- 
~Vinod