diff mbox series

[v3,2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

Message ID 20201208210941.2177-3-huobean@gmail.com (mailing list archive)
State Superseded
Headers show
Series Three changes for UFS WriteBooster | expand

Commit Message

Bean Huo Dec. 8, 2020, 9:09 p.m. UTC 
From: Bean Huo <beanhuo@micron.com>

According to the JEDEC UFS 3.1 Spec, If fWriteBoosterBufferFlushDuringHibernate
is set to one, the device flushes the WriteBooster Buffer data automatically
whenever the link enters the hibernate (HIBERN8) state. While the flushing
operation is in progress, the device should be kept in Active power mode.
Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
with its programming failure. Even this failure is less likely to occur, but
still it is possible.
This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate setting,
keep the device as "active power mode" only when this flag be successfully set
to 1.

Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime suspend")
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs.h    |  2 ++
 drivers/scsi/ufs/ufshcd.c | 20 +++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

Comments

Avri Altman Dec. 9, 2020, 7:40 a.m. UTC | #1 
> From: Bean Huo <beanhuo@micron.com>
> 
> According to the JEDEC UFS 3.1 Spec, If
> fWriteBoosterBufferFlushDuringHibernate
> is set to one, the device flushes the WriteBooster Buffer data automatically
> whenever the link enters the hibernate (HIBERN8) state. While the flushing
> operation is in progress, the device should be kept in Active power mode.
> Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
> with its programming failure. Even this failure is less likely to occur, but
> still it is possible.
How about reading it on every ufshcd_wb_need_flush?

Thanks,
Avri
Bean Huo Dec. 9, 2020, 10:03 p.m. UTC | #2 
On Wed, 2020-12-09 at 07:40 +0000, Avri Altman wrote:
> > According to the JEDEC UFS 3.1 Spec, If
> > fWriteBoosterBufferFlushDuringHibernate
> > is set to one, the device flushes the WriteBooster Buffer data
> > automatically
> > whenever the link enters the hibernate (HIBERN8) state. While the
> > flushing
> > operation is in progress, the device should be kept in Active power
> > mode.
> > Currently, we set this flag during the UFSHCD probe stage, but we
> > didn't deal
> > with its programming failure. Even this failure is less likely to
> > occur, but
> > still it is possible.
> 
> How about reading it on every ufshcd_wb_need_flush?
> 
> Thanks,
> Avri


Hi Avri
I was using that way, no different from the current my way. Instead,
reading on every time will add some delay. As long as the UFS device
returns the successful, we assume that this flag has been properly
set.  so, just keeping is_hibern8_wb_flush if set, I think it is
enough.

Thanks,
Bean
Bean Huo Dec. 9, 2020, 10:08 p.m. UTC | #3 
On Wed, 2020-12-09 at 07:40 +0000, Avri Altman wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > According to the JEDEC UFS 3.1 Spec, If
> > fWriteBoosterBufferFlushDuringHibernate
> > is set to one, the device flushes the WriteBooster Buffer data
> > automatically
> > whenever the link enters the hibernate (HIBERN8) state. While the
> > flushing
> > operation is in progress, the device should be kept in Active power
> > mode.
> > Currently, we set this flag during the UFSHCD probe stage, but we
> > didn't deal
> > with its programming failure. Even this failure is less likely to
> > occur, but
> > still it is possible.

Hi, Can and Stanley
Please review this patch, and leave your comments.
or if you look good, leave your review-tag, in order that Martin can
accept this series patch easily.

Thanks,
Bean
Avri Altman Dec. 10, 2020, 7:46 a.m. UTC | #4 
> On Wed, 2020-12-09 at 07:40 +0000, Avri Altman wrote:
> > > According to the JEDEC UFS 3.1 Spec, If
> > > fWriteBoosterBufferFlushDuringHibernate
> > > is set to one, the device flushes the WriteBooster Buffer data
> > > automatically
> > > whenever the link enters the hibernate (HIBERN8) state. While the
> > > flushing
> > > operation is in progress, the device should be kept in Active power
> > > mode.
> > > Currently, we set this flag during the UFSHCD probe stage, but we
> > > didn't deal
> > > with its programming failure. Even this failure is less likely to
> > > occur, but
> > > still it is possible.
> >
> > How about reading it on every ufshcd_wb_need_flush?
> >
> > Thanks,
> > Avri
> 
> 
> Hi Avri
> I was using that way, no different from the current my way. Instead,
> reading on every time will add some delay. As long as the UFS device
> returns the successful, we assume that this flag has been properly
> set.  so, just keeping is_hibern8_wb_flush if set, I think it is
> enough.
Right.
But it is a small price, and you no longer need to worry about rare error event.
Also adding an if (fWriteBoosterBufferFlushDuringHibernate == 1) will allow some more flexibility,
e.g. shutting it off from user-space (ufs-utils), unlike today,
that it is categorically on for all platforms / devices.

Anyway, if you decided to add new capability,
Preferable to do it in a different series.

Thanks,
Avri
Bean Huo Dec. 10, 2020, 8:13 p.m. UTC | #5 
On Thu, 2020-12-10 at 07:46 +0000, Avri Altman wrote:
> > 
> 
> Right.
> But it is a small price, and you no longer need to worry about rare
> error event.
> Also adding an if (fWriteBoosterBufferFlushDuringHibernate == 1) will
> allow some more flexibility,
> e.g. shutting it off from user-space (ufs-utils), unlike today,
> that it is categorically on for all platforms / devices.
> 
> Anyway, if you decided to add new capability,
> Preferable to do it in a different series.
> 
> Thanks,
> Avri

Hi Avri
Thanks. This reminds me that ufs-bsg is a latent defect. Currently,
userspace can pass any raw UPIU commands to the UFS through ufs-bsg,
ufs-bsg is a pass-through channel. So,
fWriteBoosterBufferFlushDuringHibernate is not the only one in the
ufshcd.c can be changed by ufs-utils. any flags in the UFS can be
changed by user-space tool after UFS finishing its initialization.


This modification after the fact (Linux initialization/probe itself) is
not legal. I remembered we discussed this on the eMMC case, the same
with here that, user can change some parameters in the eMMC through
eMMC Ioctl, the user feels great, but they did a wrong thing.


Ulf Hansson: "I don't think it's worth to compensate and try
to act accordingly to cover cases when userspace has messed up."



thanks,
Bean
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index d593edb48767..311d5f7a024d 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -530,6 +530,8 @@  struct ufs_dev_info {
 	bool f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
 	bool is_lu_power_on_wp;
+	/* Indicates if flush WB buffer during hibern8 successfully enabled */
+	bool is_hibern8_wb_flush;
 	/* Maximum number of general LU supported by the UFS device */
 	u8 max_lu_supported;
 	u8 wb_dedicated_lu;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f9fefb6c7ddb..f3ba46c48383 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -283,10 +283,16 @@  static inline void ufshcd_wb_config(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
 	else
 		dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
+
 	ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
-	if (ret)
+	if (ret) {
 		dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
 			__func__, ret);
+		hba->dev_info.is_hibern8_wb_flush = false;
+	} else {
+		hba->dev_info.is_hibern8_wb_flush = true;
+	}
+
 	ufshcd_wb_toggle_flush(hba, true);
 }
 
@@ -5444,6 +5450,7 @@  static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 
 	if (!ufshcd_is_wb_allowed(hba))
 		return false;
+
 	/*
 	 * The ufs device needs the vcc to be ON to flush.
 	 * With user-space reduction enabled, it's enough to enable flush
@@ -8535,6 +8542,7 @@  static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	enum ufs_pm_level pm_lvl;
 	enum ufs_dev_pwr_mode req_dev_pwr_mode;
 	enum uic_link_state req_link_state;
+	bool hibern8;
 
 	hba->pm_op_in_progress = 1;
 	if (!ufshcd_is_shutdown_pm(pm_op)) {
@@ -8594,11 +8602,13 @@  static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		 * Hibern8, keep device power mode as "active power mode"
 		 * and VCC supply.
 		 */
+		hibern8 = req_link_state == UIC_LINK_HIBERN8_STATE ||
+			(req_link_state == UIC_LINK_ACTIVE_STATE &&
+			 ufshcd_is_auto_hibern8_enabled(hba));
+
 		hba->dev_info.b_rpm_dev_flush_capable =
-			hba->auto_bkops_enabled ||
-			(((req_link_state == UIC_LINK_HIBERN8_STATE) ||
-			((req_link_state == UIC_LINK_ACTIVE_STATE) &&
-			ufshcd_is_auto_hibern8_enabled(hba))) &&
+			hba->auto_bkops_enabled || (hibern8 &&
+			hba->dev_info.is_hibern8_wb_flush &&
 			ufshcd_wb_need_flush(hba));
 	}