linux-kernel - Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp

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: <203ffe4d-2b8e-7550-2fab-f5e3c7b5f4fe@gmail.com>
Date:   Mon, 17 May 2021 18:23:27 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Paul Fertser <fercerpav@...il.com>,
        Matt Merhar <mattmerhar@...tonmail.com>,
        Peter Geis <pgwipeout@...il.com>,
        Nicolas Chauvet <kwizart@...il.com>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>
Cc:     linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] soc/tegra: Add
 devm_tegra_core_dev_init_opp_table()

17.05.2021 17:52, Krzysztof Kozlowski пишет:
>>>> +static int tegra_core_dev_init_opp_state(struct device *dev)
>>>> +{
>>>> +	struct dev_pm_opp *opp;
>>>> +	unsigned long rate;
>>>> +	struct clk *clk;
>>>> +	int err;
>>>> +
>>>> +	clk = devm_clk_get(dev, NULL);
>>>> +	if (IS_ERR(clk)) {
>>>> +		dev_err(dev, "failed to get clk: %pe\n", clk);
>>>> +		return PTR_ERR(clk);
>>>> +	}
>>>> +
>>>> +	rate = clk_get_rate(clk);
>>>> +	if (!rate) {
>>>> +		dev_err(dev, "failed to get clk rate\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	opp = dev_pm_opp_find_freq_ceil(dev, &rate);
>>>> +
>>>> +	if (opp == ERR_PTR(-ERANGE))
>>>> +		opp = dev_pm_opp_find_freq_floor(dev, &rate);
>>>> +
>>>> +	err = PTR_ERR_OR_ZERO(opp);
>>>> +	if (err) {
>>>> +		dev_err(dev, "failed to get OPP for %ld Hz: %d\n",
>>>> +			rate, err);
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	dev_pm_opp_put(opp);
>>>> +
>>>> +	/* first dummy rate-setting initializes voltage vote */
>>>> +	err = dev_pm_opp_set_rate(dev, rate);
>>>> +	if (err) {
>>>> +		dev_err(dev, "failed to initialize OPP clock: %d\n", err);
>>>> +		return err;
>>>> +	}
>>>
>>> The devm_pm_opp_set_clkname will call clk_get(), so here you should drop
>>> the clk reference at the end. Why having it twice?
>> The devm_pm_opp_set_clkname assigns clock to the OPP table.
>>
>> The devm_clk_get() is needed for the clk_get_rate(). OPP core doesn't
>> initialize voltage vote and we need this initialization for the Tegra
>> memory drivers.
> I did not get the answer to my question. Why you need to keep the clk
> reference past this point? Why you cannot drop it after getting rate?
> 
>> The reference count of the clk will be dropped automatically once device
>> driver is released. The resource-managed helper avoids the need to care
>> about the error unwinding in the code, making it clean and easy to follow.
> I am not saying there is a leak.
> 

The clk reference is not needed past this point. It doesn't hurt to have
additional reference since this allows to make code cleaner.