Re: [PATCH v31 2/4] scsi: ufs: L2P map management for HPB read

[Can Guo]

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.