linux-kernel - Re: [PATCH v11 3/4] clk: Add rate constraints to clocks

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: <54C10C54.6070500@collabora.com>
Date:	Thu, 22 Jan 2015 15:42:28 +0100
From:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
To:	Stephen Boyd <sboyd@...eaurora.org>
CC:	linux-kernel@...r.kernel.org,
	Mike Turquette <mturquette@...aro.org>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Jonathan Corbet <corbet@....net>,
	Tony Lindgren <tony@...mide.com>,
	Russell King <linux@....linux.org.uk>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	Emilio López <emilio@...pez.com.ar>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Tero Kristo <t-kristo@...com>,
	Manuel Lauss <manuel.lauss@...il.com>,
	Alex Elder <elder@...aro.org>,
	Matt Porter <mporter@...aro.org>,
	Zhangfei Gao <zhangfei.gao@...aro.org>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Jaehoon Chung <jh80.chung@...sung.com>,
	Bintian Wang <bintian.wang@...wei.com>,
	Chao Xie <chao.xie@...vell.com>, linux-doc@...r.kernel.org,
	linux-omap@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mips@...ux-mips.org
Subject: Re: [PATCH v11 3/4] clk: Add rate constraints to clocks

On 01/22/2015 02:46 AM, Stephen Boyd wrote:
> On 01/21, Tomeu Vizoso wrote:
>> Adds a way for clock consumers to set maximum and minimum rates. This
>> can be used for thermal drivers to set minimum rates, or by misc.
>> drivers to set maximum rates to assure a minimum performance level.
>>
>> Changes the signature of the determine_rate callback by adding the
>> parameters min_rate and max_rate.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
>>
>> ---
>> v11:	* Recalculate the rate before putting the reference to clk_core
>> 	* Don't recalculate the rate when freeing the per-user clock
>> 	in the initialization error paths
>> 	* Move __clk_create_clk to be next to __clk_free_clk for more
>> 	comfortable reading
> 
> Can we do this in the previous patch where we introduce the
> function?

Ok.

>> @@ -2143,9 +2314,16 @@ struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
>>  	else
>>  		clk->owner = NULL;
>>  
>> +	INIT_HLIST_HEAD(&clk->clks);
>> +
>> +	hw->clk = __clk_create_clk(hw, NULL, NULL);
>> +
>>  	ret = __clk_init(dev, hw->clk);
>> -	if (ret)
>> +	if (ret) {
>> +		__clk_free_clk(hw->clk);
>> +		hw->clk = NULL;
>>  		return ERR_PTR(ret);
>> +	}
>>  
>>  	return hw->clk;
>>  }
>> @@ -2210,12 +2388,16 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>>  		}
>>  	}
>>  
>> +	INIT_HLIST_HEAD(&clk->clks);
>> +
>>  	hw->clk = __clk_create_clk(hw, NULL, NULL);
>>  	ret = __clk_init(dev, hw->clk);
>>  	if (!ret)
>>  		return hw->clk;
>>  
>> -	kfree(hw->clk);
>> +	__clk_free_clk(hw->clk);
>> +	hw->clk = NULL;
> 
> Shouldn't we be assigning to NULL in the previous patch (same
> comment for __clk_register)?

Agreed, though I have gone ahead and removed __clk_register completely
because AFAICS it has never been used.

>>  fail_parent_names_copy:
>>  	while (--i >= 0)
>>  		kfree(clk->parent_names[i]);
>> @@ -2420,7 +2602,14 @@ void __clk_put(struct clk *clk)
>>  	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>>  		return;
>>  
>> +	clk_prepare_lock();
>> +	hlist_del(&clk->child_node);
>> +	clk_prepare_unlock();
>> +
>> +	clk_core_set_rate(clk->core, clk->core->req_rate);
>> +
>>  	clk_core_put(clk->core);
>> +
> 
> Sad that we take the lock 3 times during __clk_put(). We should
> be able to do it only once if we have a lockless
> clk_core_set_rate() function and put the contents of
> clk_core_put() into this function. Actually we need to do that to
> be thread safe with clk->core->req_rate changing. We can call the
> same function in clk_set_rate_range() too so that we don't have
> to deal with recursive locking there.

Sweet, done.

>>  	kfree(clk);
>>  }
>>  
> 

Thanks,

Tomeu
--
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/