| 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: <SN6PR04MB4640E8EF26802A44B1F5D38AFCF70@SN6PR04MB4640.namprd04.prod.outlook.com>
Date: Wed, 18 Mar 2020 08:19:51 +0000
From: Avri Altman <Avri.Altman@....com>
To: Can Guo <cang@...eaurora.org>,
"asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
"nguyenb@...eaurora.org" <nguyenb@...eaurora.org>,
"hongwus@...eaurora.org" <hongwus@...eaurora.org>,
"rnayak@...eaurora.org" <rnayak@...eaurora.org>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"kernel-team@...roid.com" <kernel-team@...roid.com>,
"saravanak@...gle.com" <saravanak@...gle.com>,
"salyzyn@...gle.com" <salyzyn@...gle.com>
CC: 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
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?
> 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.
>
> /* 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
>
> -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