Date:   Wed, 24 Mar 2021 17:37:50 +0800
From:   Can Guo <cang@...eaurora.org>
To:     Bean Huo <huobean@...il.com>
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 2021-03-24 17:33, Bean Huo wrote:
> 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.
> 

Sure.

BTW, do you have plans to make the proposal into JEDEC specs?

Thanks,
Can Guo.

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

Powered by blists - more mailing lists