From: Bean Huo <[email protected]>
Changelog:
V--V5:
1. Add patch "docs: ABI: Add wb_on documentation for UFS sysfs"
2. Unify WB related flags with wb_* prefix (Stanley Chu)
3. Delete d_ext_ufs_feature_sup (Stanley Chu)
4. Incorporate Stanley's suggestion to patch 6/7
5. Replace scnprintf() with sysfs_emit() in 1/7 (Greg KH)
v3--v4:
1. Rebase patch on 5.11/scsi-staging
2. Add WB cleanup patches 3/6, 4/6 adn 5/6
v2--v3:
1. Change multi-line comments style in patch 1/3 (Can Guo)
v1--v2:
1. Take is_hibern8_wb_flush checkup out from function
ufshcd_wb_need_flush() in patch 2/3
2. Add UFSHCD_CAP_CLK_SCALING checkup in patch 1/3. that means
only for the platform, which doesn't support UFSHCD_CAP_CLK_SCALING,
can control WB through "wb_on".
Bean Huo (7):
scsi: ufs: Add "wb_on" sysfs node to control WB on/off
docs: ABI: Add wb_on documentation for UFS sysfs
scsi: ufs: Changes comment in the function ufshcd_wb_probe()
scsi: ufs: Remove two WB related fields from struct ufs_dev_info
scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
scsi: ufs: Cleanup WB buffer flush toggle implementation
scsi: ufs: Keep device active mode only
fWriteBoosterBufferFlushDuringHibernate == 1
Documentation/ABI/testing/sysfs-driver-ufs | 8 ++
drivers/scsi/ufs/ufs-sysfs.c | 41 +++++++
drivers/scsi/ufs/ufs.h | 30 ++---
drivers/scsi/ufs/ufshcd.c | 124 +++++++++------------
drivers/scsi/ufs/ufshcd.h | 6 +-
5 files changed, 122 insertions(+), 87 deletions(-)
--
2.17.1
From: Bean Huo <[email protected]>
Adds UFS sysfs documentation for new entry wb_on.
Signed-off-by: Bean Huo <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-ufs | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index d1a352194d2e..5a70d541b2f2 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1019,3 +1019,11 @@ Contact: Asutosh Das <[email protected]>
Description: This entry shows the configured size of WriteBooster buffer.
0400h corresponds to 4GB.
The file is read only.
+
+What: /sys/bus/platform/drivers/ufshcd/*/wb_on
+Date: December 2020
+Contact: Bean Huo <[email protected]>
+Description: This sets or displays whether UFS WriteBooster is enabled. Echo
+ 0 into this file to disable UFS WriteBooster or 1 to enable it. Note,
+ this is only allowed for the platform doesen't support
+ UFSHCD_CAP_CLK_SCALING.
--
2.17.1
From: Bean Huo <[email protected]>
Currently UFS WriteBooster driver uses clock scaling up/down to set
WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
WB will be always on. Provide a sysfs attribute to enable/disable WB
during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
drivers/scsi/ufs/ufshcd.c | 3 +--
drivers/scsi/ufs/ufshcd.h | 2 ++
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7eef6a..f3ca3d6b82c4 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
return count;
}
+static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", hba->wb_enabled);
+}
+
+static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ unsigned int wb_enable;
+ ssize_t res;
+
+ if (ufshcd_is_clkscaling_supported(hba)) {
+ /*
+ * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB
+ * on/off will be done while clock scaling up/down.
+ */
+ dev_warn(dev, "To control WB through wb_on is not allowed!\n");
+ return -EOPNOTSUPP;
+ }
+ if (!ufshcd_is_wb_allowed(hba))
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &wb_enable))
+ return -EINVAL;
+
+ if (wb_enable != 0 && wb_enable != 1)
+ return -EINVAL;
+
+ pm_runtime_get_sync(hba->dev);
+ res = ufshcd_wb_ctrl(hba, wb_enable);
+ pm_runtime_put_sync(hba->dev);
+
+ return res < 0 ? res : count;
+}
+
static DEVICE_ATTR_RW(rpm_lvl);
static DEVICE_ATTR_RO(rpm_target_dev_state);
static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -196,6 +235,7 @@ static DEVICE_ATTR_RW(spm_lvl);
static DEVICE_ATTR_RO(spm_target_dev_state);
static DEVICE_ATTR_RO(spm_target_link_state);
static DEVICE_ATTR_RW(auto_hibern8);
+static DEVICE_ATTR_RW(wb_on);
static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
&dev_attr_rpm_lvl.attr,
@@ -205,6 +245,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
&dev_attr_spm_target_dev_state.attr,
&dev_attr_spm_target_link_state.attr,
&dev_attr_auto_hibern8.attr,
+ &dev_attr_wb_on.attr,
NULL
};
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add25a7e..5e1dcf4de67e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -246,7 +246,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
@@ -5351,7 +5350,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
__func__, err);
}
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
{
int ret;
u8 index;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9bb5f0ed4124..2a97006a2c93 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
u8 *desc_buff, int *buff_len,
enum query_opcode desc_op);
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
+
/* Wrapper functions for safely calling variant operations */
static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
{
--
2.17.1
Hi Bean,
On Wed, 2020-12-16 at 00:05 +0100, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> Currently UFS WriteBooster driver uses clock scaling up/down to set
> WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
> WB will be always on. Provide a sysfs attribute to enable/disable WB
> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
>
> Reviewed-by: Avri Altman <[email protected]>
> Reviewed-by: Stanley Chu <[email protected]>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
> drivers/scsi/ufs/ufshcd.c | 3 +--
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..f3ca3d6b82c4 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
> return count;
> }
>
> +static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", hba->wb_enabled);
> +}
> +
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + unsigned int wb_enable;
> + ssize_t res;
> +
> + if (ufshcd_is_clkscaling_supported(hba)) {
> + /*
> + * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB
> + * on/off will be done while clock scaling up/down.
> + */
> + dev_warn(dev, "To control WB through wb_on is not allowed!\n");
> + return -EOPNOTSUPP;
> + }
> + if (!ufshcd_is_wb_allowed(hba))
> + return -EOPNOTSUPP;
> +
> + if (kstrtouint(buf, 0, &wb_enable))
> + return -EINVAL;
> +
> + if (wb_enable != 0 && wb_enable != 1)
> + return -EINVAL;
> +
> + pm_runtime_get_sync(hba->dev);
> + res = ufshcd_wb_ctrl(hba, wb_enable);
May this operation race with UFS shutdown flow?
To be more clear, ufshcd_wb_ctrl() here may be executed after host clock
is disabled by shutdown flow?
If yes, we need to avoid it.
Thanks,
Stanley Chu
> + pm_runtime_put_sync(hba->dev);
> +
> + return res < 0 ? res : count;
> +}
> +
> static DEVICE_ATTR_RW(rpm_lvl);
> static DEVICE_ATTR_RO(rpm_target_dev_state);
> static DEVICE_ATTR_RO(rpm_target_link_state);
> @@ -196,6 +235,7 @@ static DEVICE_ATTR_RW(spm_lvl);
> static DEVICE_ATTR_RO(spm_target_dev_state);
> static DEVICE_ATTR_RO(spm_target_link_state);
> static DEVICE_ATTR_RW(auto_hibern8);
> +static DEVICE_ATTR_RW(wb_on);
>
> static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
> &dev_attr_rpm_lvl.attr,
> @@ -205,6 +245,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
> &dev_attr_spm_target_dev_state.attr,
> &dev_attr_spm_target_link_state.attr,
> &dev_attr_auto_hibern8.attr,
> + &dev_attr_wb_on.attr,
> NULL
> };
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e221add25a7e..5e1dcf4de67e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -246,7 +246,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
> static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
> static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
> static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
> static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
> static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
> static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
> @@ -5351,7 +5350,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
> __func__, err);
> }
>
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> {
> int ret;
> u8 index;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 9bb5f0ed4124..2a97006a2c93 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
> u8 *desc_buff, int *buff_len,
> enum query_opcode desc_op);
>
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
> +
> /* Wrapper functions for safely calling variant operations */
> static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
> {
On 2020-12-22 14:08, Stanley Chu wrote:
> Hi Bean,
>
> On Wed, 2020-12-16 at 00:05 +0100, Bean Huo wrote:
>> From: Bean Huo <[email protected]>
>>
>> Currently UFS WriteBooster driver uses clock scaling up/down to set
>> WB on/off, for the platform which doesn't support
>> UFSHCD_CAP_CLK_SCALING,
>> WB will be always on. Provide a sysfs attribute to enable/disable WB
>> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS
>> WB.
>>
>> Reviewed-by: Avri Altman <[email protected]>
>> Reviewed-by: Stanley Chu <[email protected]>
>> Signed-off-by: Bean Huo <[email protected]>
>> ---
>> drivers/scsi/ufs/ufs-sysfs.c | 41
>> ++++++++++++++++++++++++++++++++++++
>> drivers/scsi/ufs/ufshcd.c | 3 +--
>> drivers/scsi/ufs/ufshcd.h | 2 ++
>> 3 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c
>> b/drivers/scsi/ufs/ufs-sysfs.c
>> index 08e72b7eef6a..f3ca3d6b82c4 100644
>> --- a/drivers/scsi/ufs/ufs-sysfs.c
>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
>> @@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device
>> *dev,
>> return count;
>> }
>>
>> +static ssize_t wb_on_show(struct device *dev, struct device_attribute
>> *attr,
>> + char *buf)
>> +{
>> + struct ufs_hba *hba = dev_get_drvdata(dev);
>> +
>> + return sysfs_emit(buf, "%d\n", hba->wb_enabled);
>> +}
>> +
>> +static ssize_t wb_on_store(struct device *dev, struct
>> device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct ufs_hba *hba = dev_get_drvdata(dev);
>> + unsigned int wb_enable;
>> + ssize_t res;
>> +
>> + if (ufshcd_is_clkscaling_supported(hba)) {
>> + /*
>> + * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB
>> + * on/off will be done while clock scaling up/down.
>> + */
>> + dev_warn(dev, "To control WB through wb_on is not allowed!\n");
>> + return -EOPNOTSUPP;
>> + }
>> + if (!ufshcd_is_wb_allowed(hba))
>> + return -EOPNOTSUPP;
>> +
>> + if (kstrtouint(buf, 0, &wb_enable))
>> + return -EINVAL;
>> +
>> + if (wb_enable != 0 && wb_enable != 1)
>> + return -EINVAL;
>> +
>> + pm_runtime_get_sync(hba->dev);
>> + res = ufshcd_wb_ctrl(hba, wb_enable);
>
> May this operation race with UFS shutdown flow?
>
> To be more clear, ufshcd_wb_ctrl() here may be executed after host
> clock
> is disabled by shutdown flow?
>
> If yes, we need to avoid it.
I have the same doubt - can user still access sysfs nodes after system
starts to run shutdown routines? If yes, then we need to remove all UFS
sysfs nodes in ufshcd_shutdown().
Thanks,
Can Guo.
>
> Thanks,
> Stanley Chu
>
>> + pm_runtime_put_sync(hba->dev);
>> +
>> + return res < 0 ? res : count;
>> +}
>> +
>> static DEVICE_ATTR_RW(rpm_lvl);
>> static DEVICE_ATTR_RO(rpm_target_dev_state);
>> static DEVICE_ATTR_RO(rpm_target_link_state);
>> @@ -196,6 +235,7 @@ static DEVICE_ATTR_RW(spm_lvl);
>> static DEVICE_ATTR_RO(spm_target_dev_state);
>> static DEVICE_ATTR_RO(spm_target_link_state);
>> static DEVICE_ATTR_RW(auto_hibern8);
>> +static DEVICE_ATTR_RW(wb_on);
>>
>> static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>> &dev_attr_rpm_lvl.attr,
>> @@ -205,6 +245,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[]
>> = {
>> &dev_attr_spm_target_dev_state.attr,
>> &dev_attr_spm_target_link_state.attr,
>> &dev_attr_auto_hibern8.attr,
>> + &dev_attr_wb_on.attr,
>> NULL
>> };
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e221add25a7e..5e1dcf4de67e 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -246,7 +246,6 @@ static inline int ufshcd_config_vreg_hpm(struct
>> ufs_hba *hba,
>> static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
>> static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
>> static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
>> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
>> static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool
>> set);
>> static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool
>> enable);
>> static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
>> @@ -5351,7 +5350,7 @@ static void
>> ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
>> __func__, err);
>> }
>>
>> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
>> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
>> {
>> int ret;
>> u8 index;
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 9bb5f0ed4124..2a97006a2c93 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba
>> *hba,
>> u8 *desc_buff, int *buff_len,
>> enum query_opcode desc_op);
>>
>> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
>> +
>> /* Wrapper functions for safely calling variant operations */
>> static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
>> {
On Tue, 2020-12-22 at 14:08 +0800, Stanley Chu wrote:
> > + if (kstrtouint(buf, 0, &wb_enable))
> > + return -EINVAL;
> > +
> > + if (wb_enable != 0 && wb_enable != 1)
> > + return -EINVAL;
> > +
> > + pm_runtime_get_sync(hba->dev);
> > + res = ufshcd_wb_ctrl(hba, wb_enable);
>
> May this operation race with UFS shutdown flow?
>
> To be more clear, ufshcd_wb_ctrl() here may be executed after host
> clock
> is disabled by shutdown flow?
>
> If yes, we need to avoid it.
>
> Thanks,
> Stanley Chu
Yes, it is quite possible, but who will mess up this on purpose?
ok, to counterbalance our concerns, I can add checkup:
/* UFS device & link must be active before we change WB status */
if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba))
return -EINVAL;
how do you think about?
Bean
On Tue, 2020-12-22 at 14:12 +0800, Can Guo wrote:
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (kstrtouint(buf, 0, &wb_enable))
> > > + return -EINVAL;
> > > +
> > > + if (wb_enable != 0 && wb_enable != 1)
> > > + return -EINVAL;
> > > +
> > > + pm_runtime_get_sync(hba->dev);
> > > + res = ufshcd_wb_ctrl(hba, wb_enable);
> >
> > May this operation race with UFS shutdown flow?
> >
> > To be more clear, ufshcd_wb_ctrl() here may be executed after host
> > clock
> > is disabled by shutdown flow?
> >
> > If yes, we need to avoid it.
>
> I have the same doubt - can user still access sysfs nodes after
> system
> starts to run shutdown routines? If yes, then we need to remove all
> UFS
> sysfs nodes in ufshcd_shutdown().
>
No, we shouldn't do in this way, user space complains this. I think
the nodes in the sysfs can be shileded write, but the nodes shouldn't
be flash of its presence frequently.
Thanks,
Bean
> Thanks,
>
> Can Guo.
On Tue, 2020-12-22 at 21:50 +0100, Bean Huo wrote:
> >
> > May this operation race with UFS shutdown flow?
> >
> > To be more clear, ufshcd_wb_ctrl() here may be executed after host
> > clock
> > is disabled by shutdown flow?
> >
> > If yes, we need to avoid it.
> >
> > Thanks,
> > Stanley Chu
>
> Yes, it is quite possible, but who will mess up this on purpose?
>
> ok, to counterbalance our concerns, I can add checkup:
>
>
> /* UFS device & link must be active before we change WB status */
> if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba))
> return -EINVAL;
>
>
> how do you think about?
>
> Bean
seems there are only auto_hibern8 and wb will issue command to UFS
device. if you think auto_hibern8 also needs this protection.
I will add in the next version patch, otherwise, just add this checkup
in the WB.
Look forward to your feedback!
Thanks,
Bean
On Tue, 2020-12-22 at 21:57 +0100, Bean Huo wrote:
> > > May this operation race with UFS shutdown flow?
> > >
> > > To be more clear, ufshcd_wb_ctrl() here may be executed after
> > > host
> > > clock
> > > is disabled by shutdown flow?
> > >
> > > If yes, we need to avoid it.
> >
> > I have the same doubt - can user still access sysfs nodes after
> > system
> > starts to run shutdown routines? If yes, then we need to remove all
> > UFS
> > sysfs nodes in ufshcd_shutdown().
> >
>
> No, we shouldn't do in this way, user space complains this. I think
> the nodes in the sysfs can be shileded write, but the nodes shouldn't
> be flash of its presence frequently.
>
> Thanks,
> Bean
>
>
> > Thanks,
> >
> > Can Guo.
Hi Can
Got your point, you don't want user space to interrupt UFS by sysyfs if
UFS is in power down mode. if this is true, insteading of removing all
sysfs node in ufshcd_shutdown, maybe just add this checkup before
accessing UFS device descriptors/flag/attributes/LU:
for example, for the device descriptor:
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-
sysfs.c
index b3bf7fca00e5..881fe1c24a9f 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -262,6 +262,9 @@ static ssize_t ufs_sysfs_read_desc_param(struct
ufs_hba *hba,
u8 desc_buf[8] = {0};
int ret;
+ if (!ufshcd_is_ufs_dev_active(hba) ||
!ufshcd_is_link_active(hba))
+ return -EACCES;
+
Bean
On 2020-12-23 06:11, Bean Huo wrote:
> On Tue, 2020-12-22 at 21:57 +0100, Bean Huo wrote:
>> > > May this operation race with UFS shutdown flow?
>> > >
>> > > To be more clear, ufshcd_wb_ctrl() here may be executed after
>> > > host
>> > > clock
>> > > is disabled by shutdown flow?
>> > >
>> > > If yes, we need to avoid it.
>> >
>> > I have the same doubt - can user still access sysfs nodes after
>> > system
>> > starts to run shutdown routines? If yes, then we need to remove all
>> > UFS
>> > sysfs nodes in ufshcd_shutdown().
>> >
>>
>> No, we shouldn't do in this way, user space complains this. I think
>> the nodes in the sysfs can be shileded write, but the nodes shouldn't
>> be flash of its presence frequently.
>>
>> Thanks,
>> Bean
>>
>>
>> > Thanks,
>> >
>> > Can Guo.
>
>
> Hi Can
> Got your point, you don't want user space to interrupt UFS by sysyfs if
> UFS is in power down mode. if this is true, insteading of removing all
> sysfs node in ufshcd_shutdown, maybe just add this checkup before
> accessing UFS device descriptors/flag/attributes/LU:
>
> for example, for the device descriptor:
>
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-
> sysfs.c
> index b3bf7fca00e5..881fe1c24a9f 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -262,6 +262,9 @@ static ssize_t ufs_sysfs_read_desc_param(struct
> ufs_hba *hba,
> u8 desc_buf[8] = {0};
> int ret;
>
> + if (!ufshcd_is_ufs_dev_active(hba) ||
> !ufshcd_is_link_active(hba))
> + return -EACCES;
> +
>
> Bean
First of all, this check is not helping at all, during
ufshcd_shutdown(),
both the link and dev can be active for a moment, so this check is not
helping the race condition. And assume you come up with a better check,
you want to add the check everywhere? You must have noticed the fix to
the func ufshcd_clk_gate_enable_store() from Jaegeuk Kim. So, don't
expect any luck from user space, so long the sysfs nodes are available,
users can grasp them and even stress them just for testing purposes.
I don't see why removing the sysfs nodes during ufshcd_shutdown() is a
concern to customer - we should do whatever is needed to protect LLD
contexts from user space intervene. What do you think?
Can Guo.
Thanks,
Can Guo.
On Wed, 2020-12-23 at 09:31 +0800, Can Guo wrote:
> I don't see why removing the sysfs nodes during ufshcd_shutdown() is
> a
> concern to customer - we should do whatever is needed to protect LLD
> contexts from user space intervene. What do you think?
The sysfs nodes can be removed only when the device is remvoed.
Bean
On Wed, 2020-12-23 at 09:31 +0800, Can Guo wrote:
> First of all, this check is not helping at all, during
> ufshcd_shutdown(),
> both the link and dev can be active for a moment, so this check is
> not
> helping the race condition.
yes, This checkup doesn't fix race, it is to address your remove of
sysfs nodes in the ufshcd_shutdown().
Bean
On Wed, 2020-12-23 at 09:31 +0800, Can Guo wrote:
> And assume you come up with a better check,
> you want to add the check everywhere? You must have noticed the fix
> to
> the func ufshcd_clk_gate_enable_store() from Jaegeuk Kim.
Do you mean lock spin_lock_irqsave(hba->host->host_lock, flags)?
It can completely fix race issue, but it is different with here.
ufshcd_clkgate_enable_store() doesn't call ufshcd_hold().
If you want using this lock, we should change ufshcd_hold() and
ufshcd_query_*().
Bean
On Tue, 2020-12-22 at 14:08 +0800, Stanley Chu wrote:
> Hi Bean,
>
> On Wed, 2020-12-16 at 00:05 +0100, Bean Huo wrote:
> > From: Bean Huo <[email protected]>
> >
> > Currently UFS WriteBooster driver uses clock scaling up/down to set
> > WB on/off, for the platform which doesn't support
> > UFSHCD_CAP_CLK_SCALING,
> > WB will be always on. Provide a sysfs attribute to enable/disable
> > WB
> > during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable
> > UFS WB.
> >
> > Reviewed-by: Avri Altman <[email protected]>
> > Reviewed-by: Stanley Chu <[email protected]>
> > Signed-off-by: Bean Huo <[email protected]>
> > ---
> > drivers/scsi/ufs/ufs-sysfs.c | 41
> > ++++++++++++++++++++++++++++++++++++
> > drivers/scsi/ufs/ufshcd.c | 3 +--
> > drivers/scsi/ufs/ufshcd.h | 2 ++
> > 3 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-
> > sysfs.c
> > index 08e72b7eef6a..f3ca3d6b82c4 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct
> > device *dev,
> > return count;
> > }
> >
> > +static ssize_t wb_on_show(struct device *dev, struct
> > device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", hba->wb_enabled);
> > +}
> > +
> > +static ssize_t wb_on_store(struct device *dev, struct
> > device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > + unsigned int wb_enable;
> > + ssize_t res;
> > +
> > + if (ufshcd_is_clkscaling_supported(hba)) {
> > + /*
> > + * If the platform supports UFSHCD_CAP_CLK_SCALING,
> > turn WB
> > + * on/off will be done while clock scaling up/down.
> > + */
> > + dev_warn(dev, "To control WB through wb_on is not
> > allowed!\n");
> > + return -EOPNOTSUPP;
> > + }
> > + if (!ufshcd_is_wb_allowed(hba))
> > + return -EOPNOTSUPP;
> > +
> > + if (kstrtouint(buf, 0, &wb_enable))
> > + return -EINVAL;
> > +
> > + if (wb_enable != 0 && wb_enable != 1)
> > + return -EINVAL;
> > +
> > + pm_runtime_get_sync(hba->dev);
> > + res = ufshcd_wb_ctrl(hba, wb_enable);
>
> May this operation race with UFS shutdown flow?
>
> To be more clear, ufshcd_wb_ctrl() here may be executed after host
> clock
> is disabled by shutdown flow?
>
> If yes, we need to avoid it.
>
> Thanks,
> Stanley Chu
>
Hi Stanley and Can
I just sent a new patch to address this issue, let's discusss in that
patch set, I added PM maintainer in the mail as well to help us address
concern about pm_runtime_get_sync and shutdown flow. If that way can
fix this issue, then I will update this patch again.
one note:
I didn't add "if (!ufshcd_is_ufs_dev_active(hba) ||
!ufshcd_is_link_active(hba))" checkup, since sysfs node access is a
normal request as well, UFS driver should give correct response as
possbile as it can, even if device/link is lower power mode.
Thanks,
Bean