Messages in this thread | | | Date | Wed, 24 Mar 2021 17:37:50 +0800 | From | Can Guo <> | 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.
| |