linux-kernel - Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare

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]

Date:   Mon, 25 Nov 2019 14:50:56 +0100
From:   Marc Gonzalez <marc.w.gonzalez@...e.fr>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        linux-clk <linux-clk@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

Doh! Your reply never made it to my inbox, and I never thought to check
the mailing list...

On 15/07/2019 23:46, Bjorn Andersson wrote:

> On Mon 15 Jul 08:34 PDT 2019, Marc Gonzalez wrote:
> 
> [..]
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index c0990703ce54..5e85548357c0 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_prepare);
>>  
>> +static void unprepare(void *clk)
> 
> This deserves a less generic name.

Fair enough. Though it's only because of C's function pointer idiosyncrasies
that a function wrapper is even needed.


> clk_enable() is used in code that can't sleep, in what scenario do you
> envision it being useful to enable a clock from such region until devres
> cleans up the associated device?

The use-case I had in mind was
"Device drivers that call
1) clk_prepare_enable from probe()
2) clk_disable_unprepare() in remove()"

(Russell King has pointed out the short-comings of such an approach
in a different sub-thread.)


>> +int devm_clk_prepare(struct device *dev, struct clk *clk);
>> +int devm_clk_enable(struct device *dev, struct clk *clk);
>> +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
> 
> devm_clk_prepare_enable() sounds very useful, devm_clk_prepare() might
> be useful, so keep those and drop devm_clk_enable().

Oooh, I think I understand what you mean...

I saw clk_prepare_enable() defined as clk_prepare() + clk_enable(),
and figured I'd define devm_clk_prepare_enable() as
devm_clk_prepare() + devm_clk_enable() without realizing that
devm_clk_enable() made no sense.

Solution: drop devm_clk_enable() from include/linux/clk.h
Consequence devm_clk_prepare_enable() cannot be static inline,
but that may not be a big deal...

Regards.