| 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: <351fce93-dd1c-4220-a141-71077dd38217@quicinc.com>
Date: Mon, 20 Jan 2025 20:11:57 +0800
From: Ziqi Chen <quic_ziqichen@...cinc.com>
To: Avri Altman <Avri.Altman@....com>,
"quic_cang@...cinc.com"
<quic_cang@...cinc.com>,
"bvanassche@....org" <bvanassche@....org>,
"mani@...nel.org" <mani@...nel.org>,
"beanhuo@...ron.com"
<beanhuo@...ron.com>,
"junwoo80.lee@...sung.com" <junwoo80.lee@...sung.com>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"quic_nguyenb@...cinc.com" <quic_nguyenb@...cinc.com>,
"quic_nitirawa@...cinc.com" <quic_nitirawa@...cinc.com>,
"quic_rampraka@...cinc.com" <quic_rampraka@...cinc.com>
CC: "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
Alim Akhtar
<alim.akhtar@...sung.com>,
"James E.J. Bottomley"
<James.Bottomley@...senPartnership.com>,
Peter Wang
<peter.wang@...iatek.com>,
Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org>,
Andrew Halaney <ahalaney@...hat.com>,
Maramaina Naresh <quic_mnaresh@...cinc.com>,
Eric Biggers
<ebiggers@...gle.com>, Minwoo Im <minwoo.im@...sung.com>,
open list
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/8] scsi: ufs: core: Toggle Write Booster during clock
scaling base on gear speed
Hi Avri,
Thanks for your comment.
On 1/16/2025 9:27 PM, Avri Altman wrote:
>>
>> From: Can Guo <quic_cang@...cinc.com>
>>
>> During clock scaling, Write Booster is toggled on or off based on whether the
>> clock is scaled up or down. However, with OPP V2 powered multi-level gear
>> scaling, the gear can be scaled amongst multiple gear speeds, e.g., it may
>> scale down from G5 to G4, or from G4 to G2. To provide flexibilities, add a
>> new field for clock scaling such that during clock scaling Write Booster can be
>> enabled or disabled based on gear speeds but not based on scaling up or
>> down.
>>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@...cinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@...cinc.com>
>> Signed-off-by: Can Guo <quic_cang@...cinc.com>
>> ---
>> drivers/ufs/core/ufshcd.c | 17 ++++++++++++-----
>> include/ufs/ufshcd.h | 3 +++
>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
>> 721bf9d1a356..31ebf267135b 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1395,13 +1395,17 @@ static int ufshcd_clock_scaling_prepare(struct
>> ufs_hba *hba, u64 timeout_us)
>> return ret;
>> }
>>
>> -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool
>> scale_up)
>> +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int
>> +err)
>> {
>> up_write(&hba->clk_scaling_lock);
>>
>> - /* Enable Write Booster if we have scaled up else disable it */
>> - if (ufshcd_enable_wb_if_scaling_up(hba) && !err)
>> - ufshcd_wb_toggle(hba, scale_up);
>> + /* Enable Write Booster if current gear requires it else disable it */
>> + if (ufshcd_enable_wb_if_scaling_up(hba) && !err) {
>> + bool wb_en;
> Can be initialized?
>
In this code, the wb_en variable does not need to be explicitly
initialized. This is because within the conditional statement, wb_en is
assigned a value based on the comparison between hba->pwr_info.gear_rx
and hba->clk_scaling.wb_gear. Therefore, regardless of the condition,
wb_en will be assigned either true or false.
However, it is a good practice to ensure that wb_en is correctly
assigned in all possible code paths. Thanks for your suggestion, I will
initialize it as "False" in next version.
>> +
>> + wb_en = hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear ? true
>> : false;
>
> If (wb != hba->dev_info.wb_enabled)
>> + ufshcd_wb_toggle(hba, wb_en);
>> + }
> Wouldn't it make sense to move the wb toggling to ufshcd_scale_gear ?
> This way you'll be able to leave the legacy on/off toggling?
>
We don't need legacy wb on/off any more. Regarding the logic of this
series, even if gear scale go to legacy branch , current wb toggle logic
can also cover it.
for example, for legacy gear scale , it only has 2 possible gear speed
mode, scal up to max gear or scale down to G1, we choose G3 as the
wb_gear toggle gear can cover legacy WB toggle.
Any more , some customer may want the WB keep ON or OFF, they can set
the wb_gear to G0 or max_gear to meet their requirement.
>>
>> mutex_unlock(&hba->wb_mutex);
>>
>> @@ -1456,7 +1460,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba
>> *hba, unsigned long freq,
>> }
>>
>> out_unprepare:
>> - ufshcd_clock_scaling_unprepare(hba, ret, scale_up);
>> + ufshcd_clock_scaling_unprepare(hba, ret);
>> return ret;
>> }
>>
>> @@ -1816,6 +1820,9 @@ static void ufshcd_init_clk_scaling(struct ufs_hba
>> *hba)
>> if (!hba->clk_scaling.min_gear)
>> hba->clk_scaling.min_gear = UFS_HS_G1;
>>
>> + if (!hba->clk_scaling.wb_gear)
>> + hba->clk_scaling.wb_gear = UFS_HS_G3;
> So you will toggle wb off on init (pwm) and on sporadic writes.
> I guess there is no harm done.
>
No , it won't toggle wb off on init. As per UFS hba probe sequence, the
timing of devfreq init is very late. The WB will be turn ON at the
early init and complete whole init sequence with high gear speed mode.
Not worry about WB would be turn OFF during init.
> Thanks,
> Avri
-Ziqi
>
>> +
>> INIT_WORK(&hba->clk_scaling.suspend_work,
>> ufshcd_clk_scaling_suspend_work);
>> INIT_WORK(&hba->clk_scaling.resume_work,
>> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index
>> 8c7c497d63d3..8e6c2eb68011 100644
>> --- a/include/ufs/ufshcd.h
>> +++ b/include/ufs/ufshcd.h
>> @@ -448,6 +448,8 @@ struct ufs_clk_gating {
>> * @resume_work: worker to resume devfreq
>> * @target_freq: frequency requested by devfreq framework
>> * @min_gear: lowest HS gear to scale down to
>> + * @wb_gear: enable Write Booster when HS gear scales above or equal to
>> it, else
>> + * disable Write Booster
>> * @is_enabled: tracks if scaling is currently enabled or not, controlled by
>> * clkscale_enable sysfs node
>> * @is_allowed: tracks if scaling is currently allowed or not, used to block
>> @@ -468,6 +470,7 @@ struct ufs_clk_scaling {
>> struct work_struct resume_work;
>> unsigned long target_freq;
>> u32 min_gear;
>> + u32 wb_gear;
>> bool is_enabled;
>> bool is_allowed;
>> bool is_initialized;
>> --
>> 2.34.1
>
Powered by blists - more mailing lists