| Message ID | 20201130181143.5739-3-huobean@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Three changes for UFS WriteBooster | expand |
> > From: Bean Huo <beanhuo@micron.com> > > Keep device power mode as active power mode and VCC supply only if > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful. Why would it fail? Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the device doesn't support wb, The check ufshcd_is_wb_allowed should suffice, isn't it? Thanks, Avri
On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote: > > > > From: Bean Huo <beanhuo@micron.com> > > > > Keep device power mode as active power mode and VCC supply only if > > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful. > Hi Avri Thanks so much taking time reiew. > Why would it fail? During the reliability testing in harsh environments, such as: EMS testing, in the high/low-temperature environment. The system would reboot itself, there will be programming failure very likely. If we assume failure will never hit, why we capture its result following with dev_err(). If you keep using your phone in a harsh environment, you will see this print message. Of course, in a normal environment, the chance of failure likes you to win a lottery, but the possibility still exists. > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the > device doesn't support wb, > The check ufshcd_is_wb_allowed should suffice, isn't it? > Tot at all, UFSHCD_CAP_WB_EN only tells us if the platform supports WB, doesn't tell us fWriteBoosterBufferFlushDuringHibernate status. Thanks, Bean
On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote: > > > > From: Bean Huo <beanhuo@micron.com> > > > > Keep device power mode as active power mode and VCC supply only if > > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful. Hi Avri Thanks so much taking time reiew. > Why would it fail? During the reliability testing in harsh environments, such as: EMS testing, in the high/low-temperature environment. The system would reboot itself, there will be programming failure very likely. If we assume failure will never hit, why we capture its result following with dev_err(). If you keep using your phone in a harsh environment, you will see this print message. Of course, in a normal environment, the chance of failure likes you to win a lottery, but the possibility still exists. > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the > device doesn't support wb, > The check ufshcd_is_wb_allowed should suffice, isn't it? > No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB, doesn't tell us fWriteBoosterBufferFlushDuringHibernate status. Thanks, Bean
> > On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote: > > > > > > From: Bean Huo <beanhuo@micron.com> > > > > > > Keep device power mode as active power mode and VCC supply only if > > > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful. > > Hi Avri > Thanks so much taking time reiew. > > > Why would it fail? > > During the reliability testing in harsh environments, such as: > EMS testing, in the high/low-temperature environment. The system would > reboot itself, there will be programming failure very likely. > If we assume failure will never hit, why we capture its result > following with dev_err(). If you keep using your phone in a harsh > environment, you will see this print message. > > Of course, in a normal environment, the chance of failure likes you to > win a lottery, but the possibility still exists. Exactly. Hence we need-not any extra logic protecting device management command failures. if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN is set, one should expect that any other functionality would work. Otherwise, any non-standard behavior should be added with a quirk. Thanks, Avri > > > > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the > > device doesn't support wb, > > The check ufshcd_is_wb_allowed should suffice, isn't it? > > > > No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB, > doesn't tell us fWriteBoosterBufferFlushDuringHibernate status. > > Thanks, > Bean >
On Thu, 2020-12-03 at 10:46 +0000, Avri Altman wrote: > > > > From: Bean Huo <beanhuo@micron.com> > > > > > > > > Keep device power mode as active power mode and VCC supply only > > > > if > > > > fWriteBoosterBufferFlushDuringHibernate setting 1 is > > > > successful. > > > > Hi Avri > > Thanks so much taking time reiew. > > > > > Why would it fail? > > > > During the reliability testing in harsh environments, such as: > > EMS testing, in the high/low-temperature environment. The system > > would > > reboot itself, there will be programming failure very likely. > > If we assume failure will never hit, why we capture its result > > following with dev_err(). If you keep using your phone in a harsh > > environment, you will see this print message. > > > > Of course, in a normal environment, the chance of failure likes you > > to > > win a lottery, but the possibility still exists. > > Exactly. so, you agree the possiblity of failure exists. > Hence we need-not any extra logic protecting device management > command failures. what extra logic? > > if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN is > set, UFSHCD_CAP_WB_EN set is DRAM level. still in the cache. > one should expect that any other functionality would work. > No, The programming will consume more power than reading, the later setting will more possbile fail than reading. > Otherwise, any non-standard behavior should be added with a quirk. > NO, this is not what is standard or non-standard. This is independent of UFS device/controller. It is a software design. IMO, we didn't deal with programming status that is a potential bug. If having to impose to a component, do you think should be controller or device? Instead of addin a quirk, I prefer dropping this patch. > Thanks, > Avri > > > > > > > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the > > > device doesn't support wb, > > > The check ufshcd_is_wb_allowed should suffice, isn't it? > > > > > > > No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB, > > doesn't tell us fWriteBoosterBufferFlushDuringHibernate status. > > > > Thanks, > > Bean
> On Thu, 2020-12-03 at 10:46 +0000, Avri Altman wrote: > > > > > From: Bean Huo <beanhuo@micron.com> > > > > > > > > > > Keep device power mode as active power mode and VCC supply only > > > > > if > > > > > fWriteBoosterBufferFlushDuringHibernate setting 1 is > > > > > successful. > > > > > > Hi Avri > > > Thanks so much taking time reiew. > > > > > > > Why would it fail? > > > > > > During the reliability testing in harsh environments, such as: > > > EMS testing, in the high/low-temperature environment. The system > > > would > > > reboot itself, there will be programming failure very likely. > > > If we assume failure will never hit, why we capture its result > > > following with dev_err(). If you keep using your phone in a harsh > > > environment, you will see this print message. > > > > > > Of course, in a normal environment, the chance of failure likes you > > > to > > > win a lottery, but the possibility still exists. > > > > Exactly. > > so, you agree the possiblity of failure exists. I was more relating to the lottery part. > > > Hence we need-not any extra logic protecting device management > > command failures. > > what extra logic? > > > > > if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN is > > set, > > > UFSHCD_CAP_WB_EN set is DRAM level. still in the cache. > > > one should expect that any other functionality would work. > > > No, The programming will consume more power than reading, the > later setting will more possbile fail than reading. > > > Otherwise, any non-standard behavior should be added with a quirk. > > > > NO, this is not what is standard or non-standard. This is independent > of UFS device/controller. It is a software design. IMO, we didn't deal > with programming status that is a potential bug. If having to impose to > a component, do you think should be controller or device? Instead of > addin a quirk, I prefer dropping this patch. It seems you are adding some special treatment in case some device management command failed, A vanishingly unlikely event but a one that has significant impact over power consumption. If a device is not responding properly to some specific device management command, It should be treated accordingly. Thanks, Avri > > > > > > Thanks, > > Avri > > > > > > > > > > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the > > > > device doesn't support wb, > > > > The check ufshcd_is_wb_allowed should suffice, isn't it? > > > > > > > > > > No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB, > > > doesn't tell us fWriteBoosterBufferFlushDuringHibernate status. > > > > > > Thanks, > > > Bean
On Thu, 2020-12-03 at 12:15 +0000, Avri Altman wrote: > > so, you agree the possiblity of failure exists. > > I was more relating to the lottery part. It doesn't matter. even the possibility of winning a lottery is very low, but still there is. > > > > > Hence we need-not any extra logic protecting device management > > > command failures. > > > > what extra logic? > > > > > > > > if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN > > > is > > > set, > > > > > > UFSHCD_CAP_WB_EN set is DRAM level. still in the cache. > > > > > one should expect that any other functionality would work. > > > > > > > No, The programming will consume more power than reading, the > > later setting will more possbile fail than reading. > > > > > Otherwise, any non-standard behavior should be added with a > > > quirk. > > > > > > > NO, this is not what is standard or non-standard. This is > > independent > > of UFS device/controller. It is a software design. IMO, we didn't > > deal > > with programming status that is a potential bug. If having to > > impose to > > a component, do you think should be controller or device? Instead > > of > > addin a quirk, I prefer dropping this patch. > > It seems you are adding some special treatment in case some device > management command failed, > A vanishingly unlikely event but a one that has significant impact > over power consumption. again, there is nothing with device. Obviously, you didn't do system reliability testing in harsh environment. you don't believe this is WB driver bug. I will send my next version patch with a fix-tag. even It cannot merge. but I want to highlight it is a bug. Thanks, Bean
On 2020-12-01 02:11, Bean Huo wrote: > From: Bean Huo <beanhuo@micron.com> > > Keep device power mode as active power mode and VCC supply only if > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful. > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > drivers/scsi/ufs/ufs.h | 2 ++ > drivers/scsi/ufs/ufshcd.c | 11 ++++++++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > 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 639ba9d1ccbb..eb7a2534b072 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -285,10 +285,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); > } > > @@ -5440,6 +5446,9 @@ static bool ufshcd_wb_need_flush(struct ufs_hba > *hba) > > if (!ufshcd_is_wb_allowed(hba)) > return false; > + > + if (!hba->dev_info.is_hibern8_wb_flush) > + return false; The check is in the wrong place - even if say fWriteBoosterBufferFlushDuringHibernate is failed to be enabled, ufshcd_wb_need_flush() still needs to reflect the fact that whether the wb buffer needs to be flushed or not - it should not be decided by the flag. Thanks, Can Guo. > /* > * The ufs device needs the vcc to be ON to flush. > * With user-space reduction enabled, it's enough to enable flush
On Fri, 2020-12-04 at 11:26 +0800, Can Guo wrote: > > > > if (!ufshcd_is_wb_allowed(hba)) > > return false; > > + > > + if (!hba->dev_info.is_hibern8_wb_flush) > > + return false; > > The check is in the wrong place - even if say > fWriteBoosterBufferFlushDuringHibernate is failed to be enabled, > ufshcd_wb_need_flush() still needs to reflect the fact that whether > the wb buffer needs to be flushed or not - it should not be decided > by the flag. > Can, you are right, let me take it out from this function, and see if acceptable. Thanks, Bean > Thanks, > > Can Guo.
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 639ba9d1ccbb..eb7a2534b072 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -285,10 +285,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); } @@ -5440,6 +5446,9 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba) if (!ufshcd_is_wb_allowed(hba)) return false; + + if (!hba->dev_info.is_hibern8_wb_flush) + return false; /* * The ufs device needs the vcc to be ON to flush. * With user-space reduction enabled, it's enough to enable flush