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: <ZszXKz3RX7FJNQ05@lizhi-Precision-Tower-5810>
Date: Mon, 26 Aug 2024 15:27:39 -0400
From: Frank Li <Frank.li@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Liao Yuanhong <liaoyuanhong@...o.com>, vkoul@...nel.org,
	linux-arm-kernel@...ts.infradead.org, dmaengine@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] dma:imx-sdma:Use devm_clk_get_enabled() helpers

On Fri, Aug 23, 2024 at 01:08:35PM +0100, Jonathan Cameron wrote:
> On Fri, 23 Aug 2024 18:19:31 +0800
> Liao Yuanhong <liaoyuanhong@...o.com> wrote:
>
> > Use devm_clk_get_enabled() instead of clk functions in imx-sdma.
> >
> > Signed-off-by: Liao Yuanhong <liaoyuanhong@...o.com>
> No.
> Consider why the clocks are adjusted where they are in existing code
> before 'cleaning' it up.
>
> > ---
> >  drivers/dma/imx-sdma.c | 57 ++++--------------------------------------
> >  1 file changed, 5 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 72299a08af44..af972a4b6ce1 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -1493,24 +1493,11 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
> >  	sdmac->event_id0 = data->dma_request;
> >  	sdmac->event_id1 = data->dma_request2;
> >
> > -	ret = clk_enable(sdmac->sdma->clk_ipg);
> > -	if (ret)
> > -		return ret;
> > -	ret = clk_enable(sdmac->sdma->clk_ahb);
> > -	if (ret)
> > -		goto disable_clk_ipg;
> > -
> >  	ret = sdma_set_channel_priority(sdmac, prio);
> >  	if (ret)
> > -		goto disable_clk_ahb;
> > +		return ret;
> >
> >  	return 0;
> > -
> > -disable_clk_ahb:
> > -	clk_disable(sdmac->sdma->clk_ahb);
> > -disable_clk_ipg:
> > -	clk_disable(sdmac->sdma->clk_ipg);
> > -	return ret;
> >  }
> >
> >  static void sdma_free_chan_resources(struct dma_chan *chan)
> > @@ -1530,9 +1517,6 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
> >  	sdmac->event_id1 = 0;
> >
> >  	sdma_set_channel_priority(sdmac, 0);
> > -
> > -	clk_disable(sdma->clk_ipg);
> > -	clk_disable(sdma->clk_ahb);
> >  }
> >
> >  static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
> > @@ -2015,14 +1999,10 @@ static void sdma_load_firmware(const struct firmware *fw, void *context)
> >  	addr = (void *)header + header->script_addrs_start;
> >  	ram_code = (void *)header + header->ram_code_start;
> >
> > -	clk_enable(sdma->clk_ipg);
> > -	clk_enable(sdma->clk_ahb);
> >  	/* download the RAM image for SDMA */
> >  	sdma_load_script(sdma, ram_code,
> >  			 header->ram_code_size,
> >  			 addr->ram_code_start_addr);
> > -	clk_disable(sdma->clk_ipg);
> > -	clk_disable(sdma->clk_ahb);
> Why do you think it is suddenly fine to leave the locks on here and it
> wasn't before?
>
> Check all the paths.
>
> >
> >  	sdma_add_scripts(sdma, addr);
> >
> > @@ -2119,13 +2099,6 @@ static int sdma_init(struct sdma_engine *sdma)
> >  	dma_addr_t ccb_phys;
> >  	int ccbsize;
> >
> > -	ret = clk_enable(sdma->clk_ipg);
> > -	if (ret)
> > -		return ret;
> > -	ret = clk_enable(sdma->clk_ahb);
> > -	if (ret)
> > -		goto disable_clk_ipg;
> > -
> >  	if (sdma->drvdata->check_ratio &&
> >  	    (clk_get_rate(sdma->clk_ahb) == clk_get_rate(sdma->clk_ipg)))
> >  		sdma->clk_ratio = 1;
> > @@ -2180,15 +2153,9 @@ static int sdma_init(struct sdma_engine *sdma)
> >  	/* Initializes channel's priorities */
> >  	sdma_set_channel_priority(&sdma->channel[0], 7);
> >
> > -	clk_disable(sdma->clk_ipg);
> > -	clk_disable(sdma->clk_ahb);
> > -
> >  	return 0;
> >
> >  err_dma_alloc:
> > -	clk_disable(sdma->clk_ahb);
> > -disable_clk_ipg:
> > -	clk_disable(sdma->clk_ipg);
> >  	dev_err(sdma->dev, "initialisation failed with %d\n", ret);
> >  	return ret;
> >  }
> > @@ -2266,33 +2233,25 @@ static int sdma_probe(struct platform_device *pdev)
> >  	if (IS_ERR(sdma->regs))
> >  		return PTR_ERR(sdma->regs);
> >
> > -	sdma->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > +	sdma->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> >  	if (IS_ERR(sdma->clk_ipg))
> >  		return PTR_ERR(sdma->clk_ipg);
> >
> > -	sdma->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> > +	sdma->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");

Supposed it should be devm_clk_get_prepared() here because below code use
clk_prepare().

Please cc to imx@...ts.linux.dev next time.

Frank

> >  	if (IS_ERR(sdma->clk_ahb))
> >  		return PTR_ERR(sdma->clk_ahb);
> >
> > -	ret = clk_prepare(sdma->clk_ipg);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = clk_prepare(sdma->clk_ahb);
> > -	if (ret)
> > -		goto err_clk;
> > -
> >  	ret = devm_request_irq(&pdev->dev, irq, sdma_int_handler, 0,
> >  				dev_name(&pdev->dev), sdma);
> >  	if (ret)
> > -		goto err_irq;
> > +		return ret;
> >
> >  	sdma->irq = irq;
> >
> >  	sdma->script_addrs = kzalloc(sizeof(*sdma->script_addrs), GFP_KERNEL);
> >  	if (!sdma->script_addrs) {
> >  		ret = -ENOMEM;
> > -		goto err_irq;
> > +		return ret;
> >  	}
> >
> >  	/* initially no scripts available */
> > @@ -2407,10 +2366,6 @@ static int sdma_probe(struct platform_device *pdev)
> >  	dma_async_device_unregister(&sdma->dma_device);
> >  err_init:
> >  	kfree(sdma->script_addrs);
> > -err_irq:
> > -	clk_unprepare(sdma->clk_ahb);
> > -err_clk:
> > -	clk_unprepare(sdma->clk_ipg);
> >  	return ret;
> >  }
> >
> > @@ -2422,8 +2377,6 @@ static void sdma_remove(struct platform_device *pdev)
> >  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
> >  	dma_async_device_unregister(&sdma->dma_device);
> >  	kfree(sdma->script_addrs);
> > -	clk_unprepare(sdma->clk_ahb);
> > -	clk_unprepare(sdma->clk_ipg);
> >  	/* Kill the tasklet */
> >  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> >  		struct sdma_channel *sdmac = &sdma->channel[i];
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ