| 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: <d930bd05f2614092e6d8d3ef62017d89@codeaurora.org>
Date: Wed, 25 Mar 2020 16:56:37 +0800
From: Can Guo <cang@...eaurora.org>
To: Avri Altman <Avri.Altman@....com>
Cc: asutoshd@...eaurora.org, nguyenb@...eaurora.org,
hongwus@...eaurora.org, rnayak@...eaurora.org,
linux-scsi@...r.kernel.org, kernel-team@...roid.com,
saravanak@...gle.com, salyzyn@...gle.com,
Subhash Jadavani <subhashj@...eaurora.org>,
Alim Akhtar <alim.akhtar@...sung.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
Stanley Chu <stanley.chu@...iatek.com>,
Bean Huo <beanhuo@...ron.com>,
Bart Van Assche <bvanassche@....org>,
Venkat Gopalakrishnan <venkatg@...eaurora.org>,
Tomas Winkler <tomas.winkler@...el.com>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock
scaling error out path
On 2020-03-18 16:19, Avri Altman wrote:
> Hi,
>
>>
>> From: Subhash Jadavani <subhashj@...eaurora.org>
>>
>> This change introduces a func ufshcd_set_clk_freq() to explicitly
>> set clock frequency so that it can be used in reset_and_resotre path
>> and
>> in ufshcd_scale_clks(). Meanwhile, this change cleans up the clock
>> scaling
>> error out path.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@...eaurora.org>
>> Signed-off-by: Can Guo <cang@...eaurora.org>
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 2a2a63b..63aaa88f 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -855,28 +855,29 @@ static bool
>> ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>> return false;
>> }
>>
>> -static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
>> +/**
>> + * ufshcd_set_clk_freq - set UFS controller clock frequencies
>> + * @hba: per adapter instance
>> + * @scale_up: If True, set max possible frequency othewise set low
>> frequency
>> + *
>> + * Returns 0 if successful
>> + * Returns < 0 for any other errors
>> + */
>> +static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
> Personally I prefer using the convention of "__scale_clks" to describe
> the privet core of scale_clks,
> But I know that many people are against it.
>
>> {
>> int ret = 0;
>> struct ufs_clk_info *clki;
>> struct list_head *head = &hba->clk_list_head;
>> - ktime_t start = ktime_get();
>> - bool clk_state_changed = false;
>>
>> if (list_empty(head))
>> goto out;
>>
>> - ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
>> - if (ret)
>> - return ret;
>> -
>> 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;
>>
>> - clk_state_changed = true;
>> ret = clk_set_rate(clki->clk,
>> clki->max_freq);
>> if (ret) {
>> dev_err(hba->dev, "%s: %s clk
>> set rate(%dHz) failed,
>> %d\n",
>> @@ -895,7 +896,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba,
>> bool scale_up)
>> if (clki->curr_freq == clki->min_freq)
>> continue;
>>
>> - clk_state_changed = true;
>> ret = clk_set_rate(clki->clk,
>> clki->min_freq);
>> if (ret) {
>> dev_err(hba->dev, "%s: %s clk
>> set rate(%dHz) failed,
>> %d\n",
>> @@ -914,13 +914,36 @@ static int ufshcd_scale_clks(struct ufs_hba
>> *hba,
>> bool scale_up)
>> clki->name, clk_get_rate(clki->clk));
>> }
>>
>> +out:
>> + return ret;
>> +}
>> +
>> +/**
>> + * ufshcd_scale_clks - scale up or scale down UFS controller clocks
>> + * @hba: per adapter instance
>> + * @scale_up: True if scaling up and false if scaling down
>> + *
>> + * Returns 0 if successful
>> + * Returns < 0 for any other errors
>> + */
>> +static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
>> +{
>> + int ret = 0;
>> +
>> + ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ufshcd_set_clk_freq(hba, scale_up);
>> + if (ret)
>> + return ret;
>> +
>> ret = ufshcd_vops_clk_scale_notify(hba, scale_up,
>> POST_CHANGE);
>> + if (ret) {
>> + ufshcd_set_clk_freq(hba, !scale_up);
>> + return ret;
>> + }
>>
>> -out:
>> - if (clk_state_changed)
>> - trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
>> - (scale_up ? "up" : "down"),
>> - ktime_to_us(ktime_sub(ktime_get(), start)),
>> ret);
>
> Why remove the ufshcd_profile_clk_scaling trace?
>
>
Shall add it back, this is just a collection of Subhash's changes.
>> return ret;
>> }
>>
>> @@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba
>> *hba, bool scale_up)
>>
>> ret = ufshcd_clock_scaling_prepare(hba);
>> if (ret)
>> - return ret;
>> + goto out;
> Are you fixing here a hold without release?
> Should make note of that.
>
Yes, true
>>
>> /* scale down the gear before scaling down clocks */
>> if (!scale_up) {
>> ret = ufshcd_scale_gear(hba, false);
>> if (ret)
>> - goto out;
>> + goto clk_scaling_unprepare;
>> }
>>
>> ret = ufshcd_scale_clks(hba, scale_up);
>> - if (ret) {
>> - if (!scale_up)
>> - ufshcd_scale_gear(hba, true);
>> - goto out;
>> - }
>> + if (ret)
>> + goto scale_up_gear;
>>
>> /* scale up the gear after scaling up clocks */
>> if (scale_up) {
>> ret = ufshcd_scale_gear(hba, true);
>> if (ret) {
>> ufshcd_scale_clks(hba, false);
>> - goto out;
>> + goto clk_scaling_unprepare;
>> }
>> }
>>
>> - ret = ufshcd_vops_clk_scale_notify(hba, scale_up,
>> POST_CHANGE);
>> + goto clk_scaling_unprepare;
> I think you should find a way to make this function more readable.
> Adding all those "spaghetti" gotos making it even harder to read.
>
> Thanks,
> Avri
>
This is just a collection of Subhash's changes. I think the 2
clk_scaling_unprepare and out gotos make sense, but the
scale_up_gear goto is a bit vague. I will remove the
scale_up_gear goto.
Thanks,
Can Guo.
>>
>> -out:
>> +scale_up_gear:
>> + if (!scale_up)
>> + ufshcd_scale_gear(hba, true);
>> +clk_scaling_unprepare:
>> ufshcd_clock_scaling_unprepare(hba);
>> +out:
>> ufshcd_release(hba);
>> return ret;
>> }
>> @@ -6251,7 +6275,7 @@ static int ufshcd_host_reset_and_restore(struct
>> ufs_hba *hba)
>> spin_unlock_irqrestore(hba->host->host_lock, flags);
>>
>> /* scale up clocks to max frequency before full
>> reinitialization */
>> - ufshcd_scale_clks(hba, true);
>> + ufshcd_set_clk_freq(hba, true);
>>
>> err = ufshcd_hba_enable(hba);
>> if (err)
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
>> Linux Foundation Collaborative Project.
Powered by blists - more mailing lists