Message-ID: <7bfcc7d6cedd1d674172722f8cc074dac9c2a640.camel@gmail.com>
Date:   Wed, 24 Mar 2021 10:33:40 +0100
From:   Bean Huo <huobean@...il.com>
To:     Can Guo <cang@...eaurora.org>
Cc:     Avri Altman <Avri.Altman@....com>, daejun7.park@...sung.com,
        Greg KH <gregkh@...uxfoundation.org>, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, asutoshd@...eaurora.org,
        stanley.chu@...iatek.com, bvanassche@....org,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        ALIM AKHTAR <alim.akhtar@...sung.com>,
        JinHwan Park <jh.i.park@...sung.com>,
        Javier Gonzalez <javier.gonz@...sung.com>,
        Sung-Jun Park <sungjun07.park@...sung.com>,
        Jinyoung CHOI <j-young.choi@...sung.com>,
        Dukhyun Kwon <d_hyun.kwon@...sung.com>,
        Keoseong Park <keosung.park@...sung.com>,
        Jaemyung Lee <jaemyung.lee@...sung.com>,
        Jieon Seol <jieon.seol@...sung.com>
Subject: Re: [PATCH v31 2/4] scsi: ufs: L2P map management for HPB read

On Wed, 2021-03-24 at 17:24 +0800, Can Guo wrote:
> On 2021-03-24 16:37, Bean Huo wrote:
> > On Wed, 2021-03-24 at 09:45 +0800, Can Guo wrote:
> > > On 2021-03-23 20:48, Avri Altman wrote:
> > > 
> > > > > On 2021-03-23 14:37, Daejun Park wrote:
> > > > > > > On 2021-03-23 14:19, Daejun Park wrote:
> > > > > > > > > On 2021-03-23 13:37, Daejun Park wrote:
> > > > > > > > > > > On 2021-03-23 12:22, Can Guo wrote:
> > > > > > > > > > > > On 2021-03-22 17:11, Bean Huo wrote:
> > > > > > > > > > > > > On Mon, 2021-03-22 at 15:54 +0900, Daejun
> > > > > > > > > > > > > Park
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > +       switch (rsp_field->hpb_op) {
> > > > > > > > > > > > > > +       case HPB_RSP_REQ_REGION_UPDATE:
> > > > > > > > > > > > > > +               if (data_seg_len !=
> > > > > > > > > > > > > > DEV_DATA_SEG_LEN)
> > > > > > > > > > > > > > +                       dev_warn(&hpb-
> > > > > > > > > > > > > > > sdev_ufs_lu->sdev_dev,
> > > > > > > > > > > > > > +                                "%s: data
> > > > > > > > > > > > > > seg
> > > > > > > > > > > > > > length is not
> > > > > > > > > > > > > > same.\n",
> > > > > > > > > > > > > > +                                __func__);
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > ufshpb_rsp_req_region_update(hpb,
> > > > > > > > > > > > > > rsp_field);
> > > > > > > > > > > > > > +               break;
> > > > > > > > > > > > > > +       case HPB_RSP_DEV_RESET:
> > > > > > > > > > > > > > +               dev_warn(&hpb->sdev_ufs_lu-
> > > > > > > > > > > > > > > sdev_dev,
> > > > > > > > > > > > > > +                        "UFS device lost
> > > > > > > > > > > > > > HPB
> > > > > > > > > > > > > > information
> > > > > > > > > > > > > > during
> > > > > > > > > > > > > > PM.\n");
> > > > > > > > > > > > > > +               break;
> > > > > > > > > > > > > Hi Deajun,
> > > > > > > > > > > > > This series looks good to me. Just here I
> > > > > > > > > > > > > have
> > > > > > > > > > > > > one question. You
> > > > > > > > > > > > > didn't
> > > > > > > > > > > > > handle HPB_RSP_DEV_RESET, just a
> > > > > > > > > > > > > warning.  Based
> > > > > > > > > > > > > on your SS UFS,
> > > > > > > > > > > > > how
> > > > > > > > > > > > > to
> > > > > > > > > > > > > handle HPB_RSP_DEV_RESET from the host side?
> > > > > > > > > > > > > Do
> > > > > > > > > > > > > you think we
> > > > > > > > > > > > > shoud
> > > > > > > > > > > > > reset host side HPB entry as well or what
> > > > > > > > > > > > > else?
> > > > > > > > > > > > > Bean
> > > > > > > > > > > > Same question here - I am still collecting
> > > > > > > > > > > > feedbacks from flash
> > > > > > > > > > > > vendors
> > > > > > > > > > > > about
> > > > > > > > > > > > what is recommanded host behavior on reception
> > > > > > > > > > > > of
> > > > > > > > > > > > HPB Op code
> > > > > > > > > > > > 0x2,
> > > > > > > > > > > > since it
> > > > > > > > > > > > is not cleared defined in HPB2.0 specs.
> > > > > > > > > > > > Can Guo.
> > > > > > > > > > > I think the question should be asked in the
> > > > > > > > > > > HPB2.0
> > > > > > > > > > > patch, since in
> > > > > > > > > > > HPB1.0 device
> > > > > > > > > > > control mode, a HPB reset in device side does not
> > > > > > > > > > > impact anything
> > > > > > > > > > > in
> > > > > > > > > > > host side -
> > > > > > > > > > > host is not writing back any HPB entries to
> > > > > > > > > > > device
> > > > > > > > > > > anyways and HPB
> > > > > > > > > > > Read
> > > > > > > > > > > cmd with
> > > > > > > > > > > invalid HPB entries shall be treated as normal
> > > > > > > > > > > Read(10) cmd
> > > > > > > > > > > without
> > > > > > > > > > > any
> > > > > > > > > > > problems.
> > > > > > > > > > Yes, UFS device will process read command even the
> > > > > > > > > > HPB
> > > > > > > > > > entries are
> > > > > > > > > > valid or
> > > > > > > > > > not. So it is warning about read performance drop
> > > > > > > > > > by
> > > > > > > > > > dev reset.
> > > > > > > > > Yeah, but still I am 100% sure about what should host
> > > > > > > > > do
> > > > > > > > > in case of
> > > > > > > > > HPB2.0
> > > > > > > > > when it receives HPB Op code 0x2, I am waiting for
> > > > > > > > > feedbacks.
> > > > > > > > I think the host has two choices when it receives 0x2.
> > > > > > > > One is nothing on host.
> > > > > > > > The other is discarding all HPB entries in the host.
> > > > > > > > In the JEDEC HPB spec, it as follows:
> > > > > > > > When the device is powered off by the host, the device
> > > > > > > > may
> > > > > > > > restore
> > > > > > > > L2P
> > > > > > > > map
> > > > > > > > data upon power up or build from the host’s HPB READ
> > > > > > > > command.
> > > > > > > > If some UFS builds L2P map data from the host's HPB
> > > > > > > > READ
> > > > > > > > commands, we
> > > > > > > > don't
> > > > > > > > have to discard HPB entries in the host.
> > > > > > > > So I thinks there is nothing to do when it receives
> > > > > > > > 0x2.
> > > > > > > But in HPB2.0, if we do nothing to active regions in host
> > > > > > > side, host
> > > > > > > can
> > > > > > > write
> > > > > > > HPB entries (which host thinks valid, but actually
> > > > > > > invalid in
> > > > > > > device
> > > > > > > side since
> > > > > > > reset happened) back to device through HPB Write Buffer
> > > > > > > cmds
> > > > > > > (BUFFER
> > > > > > > ID
> > > > > > > = 0x2).
> > > > > > > My question is that are all UFSs OK with this?
> > > > > > Yes, it must be OK.
> > > > > > Please refer the following the HPB 2.0 spec:
> > > > > > If the HPB Entries sent by HPB WRITE BUFFER are removed by
> > > > > > the
> > > > > > device,
> > > > > > for example, because they are not consumed for a long
> > > > > > enough
> > > > > > period of
> > > > > > time,
> > > > > > then the HPB READ command for the removed HPB entries shall
> > > > > > be
> > > > > > handled
> > > > > > as a
> > > > > > normal READ command.
> > > > > No, it is talking about the subsequent HPB READ cmd sent
> > > > > after a
> > > > > HPB
> > > > > WRITE BUFFER cmd,
> > > > > but not the HPB WRITE BUFFER cmd itself...
> > > > Looks like this discussion is going the same way as we had in
> > > > host
> > > > mode.
> > > > HPB-WRITE-BUFFER 0x2, if exist,  is always a companion to HPB-
> > > > READ.
> > > > You shouldn't consider them separately.
> > > > The device is expected to handle invalid ppn by itself, and
> > > > specifically for this case,
> > > > As Daejun explained, Handle each HPB-READ (and its companion
> > > > HPB-WRITE-BUFFER) like READ10.
> > > > For device mode, doing nothing in case of dev reset, seems to
> > > > me
> > > > like
> > > > the right thing to do.
> > > 
> > > I just got some feedbacks from other flash vendors, they all
> > > commit
> > > that
> > > 
> > > their devices can work well in this scenario [1]. Some of them
> > > proposed
> > > 
> > > even complicated (maybe better) principles of handling the "HPB
> > > reset",
> > > 
> > > but since the device works well in [1], I am OK with current
> > > (simpler)
> > > 
> > > handling of "HPB reset" - in device mode doing nothing, in host
> > > mode
> > > 
> > > re-activate regions that host is trying to do a read to.
> > > 
> > > 
> > > 
> > 
> > Our suggestion on this indication 0x2:
> > 
> > 1. If current mode is device control mode, we suggest host just
> > deactivate all active regions and don't send HPB READ BUFFER
> > command to
> > device unless device indicate host to activate certain region in
> > later
> > response. In another way, it is a signal telling host to reset host
> > side L2P entry and to rebuild the L2P mapping entry in host memroy.
> > 
> > 2. If current mode is host control mode, we suggest host send HPB
> > READ
> > BUFFER command before it wants to send read command on this region,
> > rather than sending HPB READ BUFFER commands on all regions at the
> > same
> > time.
> > 
> > 
> > Bean
> 
> Hi Bean,
> 
> I got this proposal from your side too, after that I've checked with
> Leon Ge from your side and he confirmed that it is fine that host
> just
> ignores the "HPB reset" indication. We can leave it as it is as of
> now
> and revisit it if any UFS needs extra care. What do you say?
> 
> Thanks,
> Can Guo.

Hi Can,

Agree. Current handling is ok to us, but if we want to change it, we
hope it is the same with the above suggestion. We can keep current
implementation, seeing if need changes in the near future based on the
feedback or new updates in the Spec.

Thanks,
Bean



> 
> > > Thanks,
> > > 
> > > Can Guo.

Powered by blists - more mailing lists