RE: [PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Avri Altman]

> On Mon, 2020-12-07 at 08:02 +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.
> > > 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@xxxxxxxxxx>
> >
> > You've added the fixes tag,
> 
> yes,it is a bug.
> 
> >  but my previous comment is still unanswered:
> > you are adding protection to a single device management command.
> > Why this command in particular?
> > What makes it so special that it needs this extra care?
> >
> 
> see the Spec:
> "
> If fWriteBoosterBufferFlushDuringHibernate is set to one, the device
> flushes the WriteBooster Buffer data automatically whenever the link
> enters the hibernate (HIBERN8) state.
> 
> The device shall stop the flushing operation if
> fWriteBoosterBufferFlushDuringHibernate are set to zero.
> ....
> 
> "
> If fWriteBoosterBufferFlushDuringHibernate ==0, device will not flush
> WB, even if you keep device as "active mode" and LINK in hibernate
> state.
OK, so what you are actually saying, is that since we are only toggling this flag once per
link startup / recovery, in case of a failure, however rare - the host may be still keep vcc on
for nothing, as the device will do nothing in that extra wake time.  Right?

But every time ufshcd_wb_need_flush() is called we are reading some other flags/attributes?
What about them? Why not protecting them as well?

Sorry - the whole idea doesn't make any sense to me.

Thanks,
Avri
> 
> Bean
> Thanks,
>