| 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
| ||
|
Message-ID: <20220427083947epcms2p505cc251f83ae7696b2d97993ae6aef62@epcms2p5>
Date: Wed, 27 Apr 2022 17:39:47 +0900
From: Jinyoung CHOI <j-young.choi@...sung.com>
To: Po-Wen Kao <powen.kao@...iatek.com>,
ALIM AKHTAR <alim.akhtar@...sung.com>,
Avri Altman <avri.altman@....com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>,
Matthias Brugger <matthias.bgg@...il.com>,
cpgsproxy2 <cpgsproxy2@...sung.com>,
cpgsproxy3 <cpgsproxy3@...sung.com>
CC: "wsd_upstream@...iatek.com" <wsd_upstream@...iatek.com>,
"peter.wang@...iatek.com" <peter.wang@...iatek.com>,
"stanley.chu@...iatek.com" <stanley.chu@...iatek.com>,
"alice.chao@...iatek.com" <alice.chao@...iatek.com>,
"chun-hung.wu@...iatek.com" <chun-hung.wu@...iatek.com>,
"cc.chou@...iatek.com" <cc.chou@...iatek.com>,
"chaotian.jing@...iatek.com" <chaotian.jing@...iatek.com>,
"jiajie.hao@...iatek.com" <jiajie.hao@...iatek.com>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>
Subject: RE: [PATCH 1/1] scsi: ufs: add clock-scalable property for clk
scaling
> +/**
> + * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
> + * shape check.
> + * @hba: per-adapter instance
> + * @propname: name of property
> + * @cols: column count
> + * @rows: calculated row count with given cols
> + * @array: u32 array pointer of property data with propname
> + */
> +static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
> + size_t cols, size_t *rows, u32 **array)
> +{
> + struct device *dev = hba->dev;
> + struct device_node *np = dev->of_node;
> + int len = 0, ret = 0;
> + int _rows = 0;
> +
> + if (!of_get_property(np, propname, &len)) {
> + ret = -EINVAL;
> + dev_warn(dev, "%s property not specified.\n", propname);
> + goto out;
devm_kcalloc() has not been called yet.
There is no problem with calling devm_kfree(),
but wouldn't it be nice to return the error right away?
Isn't it "%s: ~~" ?
> + }
> +
> + len = len / sizeof(**array);
> + _rows = len / cols;
> + if (len % cols != 0 || !cols || !_rows) {
Wouldn't it be nice to have a condition test first?
If cols is 0, divide it by 0 and check the condition.
> + dev_warn(dev, "%s property define error.\n", propname);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
> + if (!*array) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = of_property_read_u32_array(np, propname, *array, len);
> +
> + if (ret) {
Wouldn't it be better to check the return value without a space?
> @@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>
> if (!strcmp(name, "ref_clk"))
> clki->keep_link_active = true;
> - dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> - clki->min_freq, clki->max_freq, clki->name);
> + dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
> + clki->name, clki->scalable, clki->min_freq, clki->max_freq);
> list_add_tail(&clki->list, &hba->clk_list_head);
> }
> out:
> + devm_kfree(dev, scalable);
> + devm_kfree(dev, clkfreq);
I have a question.
scalable and clkfreq are used only in this function. (I think... not managed resource but temporary)
Do you have the advantage of using devm_kcalloc instead of kcalloc?
> @@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>
> list_for_each_entry(clki, head, list) {
> if (!IS_ERR_OR_NULL(clki->clk)) {
> - if (scale_up && clki->max_freq) {
> - if (clki->curr_freq == clki->max_freq)
> - continue;
> -
> - ret = clk_set_rate(clki->clk, clki->max_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->max_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled up", clki->name,
> - clki->curr_freq,
> - clki->max_freq);
> + target_rate = scale_up ? clki->max_freq : clki->min_freq;
>
> - clki->curr_freq = clki->max_freq;
> + if (!target_rate || clki->curr_freq == target_rate)
> + continue;
>
> - } else if (!scale_up && clki->min_freq) {
> - if (clki->curr_freq == clki->min_freq)
> - continue;
> + if (!clki->scalable)
> + goto skip;
>
> - ret = clk_set_rate(clki->clk, clki->min_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->min_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled down", clki->name,
> - clki->curr_freq,
> - clki->min_freq);
> - clki->curr_freq = clki->min_freq;
> + ret = clk_set_rate(clki->clk, target_rate);
> + if (ret) {
> + dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> + __func__, clki->name,
> + target_rate, ret);
> + break;
> }
> +skip:
> + trace_ufshcd_clk_scaling(dev_name(hba->dev),
> + clki->name, scale_up,
> + clki->curr_freq,
> + target_rate);
> + clki->curr_freq = target_rate;
> }
> dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
> clki->name, clk_get_rate(clki->clk));
clki->scalable should always be confirmed before calling clk_set_rate().
Making this as a function,
I think you can get rid of goto state
and prevent the case to be called clk_set_rate() without checking the clki->scalable. :)
Thanks,
Jinyoung.
Powered by blists - more mailing lists