linux-kernel - Re: [PATCH] dma: tegra: Use runtime

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: <52A8CFD9.6030002@wwwdotorg.org>
Date:	Wed, 11 Dec 2013 13:49:29 -0700
From:	Stephen Warren <swarren@...dotorg.org>
To:	Chaitanya Bandi <bandik@...dia.com>, vinod.koul@...el.com,
	dan.j.williams@...el.com
CC:	thierry.reding@...il.com, ldewangan@...dia.com,
	dmaengine@...r.kernel.org, linux-tegra@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dma: tegra: Use runtime_pm for clk enable/disable

On 12/11/2013 01:49 AM, Chaitanya Bandi wrote:
> Used runtime_pm APIs for clock enabling/disabling.
> Made changes such that clock is not enabled during
> idle. Also moved the usage of clk prepare/unprepare
> such that they are not called in isr context.

Hmm. This is going to cause conflicts with the patch I'm taking through
the Tegra tree which converts the driver to support the standard DMA DT
bindings. Perhaps this can wait until 3.15, or perhaps we can merge the
Tegra branch back into the DMA tree to resolve the conflict.

> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c

> - * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + * Copyright (c) 2012-13, NVIDIA CORPORATION.  All rights reserved.

s/13/2013/

> @@ -580,6 +580,11 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
>  	list_add_tail(&sgreq->node, &tdc->free_sg_req);
>  
>  	/* Do not start DMA if it is going to be terminate */
> +	if (list_empty(&tdc->pending_sg_req) && (!to_terminate)) {
> +		clk_disable(tdc->tdma->dma_clk);
> +		pm_runtime_put(tdc->tdma->dev);
> +	}
> +
>  	if (to_terminate || list_empty(&tdc->pending_sg_req))
>  		return;

Don't you want to insert the new code before the comment? Otherwise,
you're separating the existing comment and code.

Here and many other places, both pm_runtime_get/put *and*
clk_enable/disable are called. Why doesn't the code *just* call
pm_runtime_get/put, and let the implementation of those APIs perform the
clock_enable/disable? That'd be a lot more typical.

Most of the new calls to pm_runtime_*()/clk_{en,dis}able() are missing
error checking.

> @@ -682,12 +687,21 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)

> +	pm_runtime_get(tdc->tdma->dev);

I think you need pm_runtime_get_sync() here to make sure the clock gets
turned on immediately? Perhaps that why...

> +	ret = clk_enable(tdc->tdma->dma_clk);

... there's also direct manipulation of the clock everywhere.

Also, shouldn't the DMA core be calling pm_runtime_get(), rather than
each individual DMA driver?
--
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/