Message-ID: <5f68a41406b157f4ed4b3765e1d8e032@codeaurora.org>
Date:   Wed, 24 Mar 2021 17:24:20 +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 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.

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

Powered by blists - more mailing lists