diff mbox series

[v5,1/7] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

Message ID 20201215230519.15158-2-huobean@gmail.com
State Superseded
Headers show
Series Several changes for UFS WriteBooster | expand

Commit Message

Bean Huo Dec. 15, 2020, 11:05 p.m. UTC
From: Bean Huo <beanhuo@micron.com>

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 <avri.altman@wdc.com>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 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(-)

Comments

Stanley Chu Dec. 22, 2020, 6:08 a.m. UTC | #1
Hi Bean,

On Wed, 2020-12-16 at 00:05 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>

> 

> 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 <avri.altman@wdc.com>

> Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

> Signed-off-by: Bean Huo <beanhuo@micron.com>

> ---

>  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)

>  {
Can Guo Dec. 22, 2020, 6:12 a.m. UTC | #2
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 <beanhuo@micron.com>

>> 

>> 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 <avri.altman@wdc.com>

>> Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

>> Signed-off-by: Bean Huo <beanhuo@micron.com>

>> ---

>>  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)

>>  {
Bean Huo Dec. 22, 2020, 8:50 p.m. UTC | #3
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
Bean Huo Dec. 22, 2020, 8:57 p.m. UTC | #4
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.
Bean Huo Dec. 22, 2020, 9:08 p.m. UTC | #5
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
Bean Huo Dec. 22, 2020, 10:11 p.m. UTC | #6
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
Can Guo Dec. 23, 2020, 1:31 a.m. UTC | #7
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.
Bean Huo Dec. 23, 2020, 8:28 a.m. UTC | #8
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
Bean Huo Dec. 23, 2020, 8:30 a.m. UTC | #9
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
Bean Huo Dec. 23, 2020, 9:11 a.m. UTC | #10
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
Bean Huo Dec. 23, 2020, 9:52 p.m. UTC | #11
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 <beanhuo@micron.com>

> > 

> > 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 <avri.altman@wdc.com>

> > Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

> > Signed-off-by: Bean Huo <beanhuo@micron.com>

> > ---

> >  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
diff mbox series

Patch

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)
 {