linux-kernel - RE: [PATCH V15 4/6] i2c: tegra: Add DMA 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: <BYAPR12MB3398BE6DA6F1E82E5F2525C2C2690@BYAPR12MB3398.namprd12.prod.outlook.com>
Date:   Fri, 8 Feb 2019 15:49:52 +0000
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Mantravadi Karthik <mkarthik@...dia.com>,
        Shardar Mohammed <smohammed@...dia.com>,
        Timo Alho <talho@...dia.com>
CC:     "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: RE: [PATCH V15 4/6] i2c: tegra: Add DMA support

> > +	ret = tegra_i2c_init_dma(i2c_dev);
> > +	if (ret < 0)
> > +		goto disable_div_clk;
> > +
> >  	ret = tegra_i2c_init(i2c_dev);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "Failed to initialize i2c controller\n"); @@ 
> > -1160,6 +1500,7 @@ static int tegra_i2c_probe(struct platform_device 
> > *pdev)
> >  disable_div_clk:
> >  	if (i2c_dev->is_multimaster_mode)
> >  		clk_disable(i2c_dev->div_clk);
> > +	tegra_i2c_release_dma(i2c_dev);
>
> Please use "release_dma:" variant that I suggested in the comment to v14 because:
>
> 1) It's just a good (and common-style in kernel) tone to unwind errors handling in the opposite order, it makes code more straight forward and helps to avoid silly mistakes.
>
> 2) It keeps the current code consistent in regards to probe error-handling.
>

Can move tegra_i2c_release_dma under the same disable_div_clk label before clk_disable

Order in probe is
	clk_prepare
	i2c_runtime_resume
	clk_enable in case of multimaster mode
	i2c_init_dma

unwinding should be order is
	release_dma
	disable_div_clk
	disable_rpm
	unprepared

Probe failure from tegra_i2c_init_dma, tegra_i2c_init, devm_request_irq, i2c_add_numbered_adapter all of them perform goto disable_div_clk

I can add tegra_i2c_release_dma before performing clock disable under same disable_div_clk so all existing disable_div_clk labels remain same.
Otherwise adding new label release_dma causes all goto disable_div_clk statement also to be changed but its same as adding release_dma under disable_div_clk above the clk_disable.

> >  
> >  disable_rpm:
> >  	pm_runtime_disable(&pdev->dev);
>
>
> [snip]
>
> Please also address my comment to the "fix maximum transfer size" patch. And please add Wolfram Sang to the CC list.
>
> Looks like v16 should be the final, so with everything being addressed:
>
> Reviewed-by: Dmitry Osipenko <digetx@...il.com>
> Tested-by: Dmitry Osipenko <digetx@...il.com>