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] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ