Please check ufshpb init flow

Please check ufshpb init flow

[정요한(JOUNG YOHAN) Mobile SE]

Hi daejun

Please check ufshpb init flow.
sending write buffer(0x03) seems spec violation (JESD220 Commands for exceptional behavior on UAC, SAM 5r05) before UAC clear (sd_probe).
Anyway hpb reset query(ufshpb_check_hpb_reset_query) seems sufficient.
Why do we need to send write buffer?

void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev)
{
out:
    /* All LUs are initialized */
    if (atomic_dec_and_test(&hba->ufshpb_dev.slave_conf_cnt))
	There seems to be a problem with this logic.
	If hpb is set on the last LUN, write buffer command is sent before sd_probe completes.
        ufshpb_hpb_lu_prepared(hba);
}

static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba)
{

    init_success = !ufshpb_check_hpb_reset_query(hba);

    shost_for_each_device(sdev, hba->host) {
        hpb = ufshpb_get_hpb_data(sdev);
        if (!hpb)
            continue;

        if (init_success) {
            ufshpb_set_state(hpb, HPB_PRESENT);
            if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0)
                queue_work(ufshpb_wq, &hpb->map_work);
            if (!hpb->is_hcm)
                ufshpb_issue_umap_all_req(hpb); 
		This seems unnecessary.
		
        } else {

Thanks 
yohan

Re: Please check ufshpb init flow

[Bean Huo]

it's for "Inactivating all HPB regions" after reboot      

scsi_add_lun()...>ufshpb_issue_umap_all_req(hpb);

if it is a cold reboot on both host side and UFS device side, it is not
necessary to issue this write buffer.


On Fri, 2021-11-19 at 02:31 +0000, 정요한(JOUNG YOHAN) Mobile SE wrote:
> Hi daejun
> 
> Please check ufshpb init flow.
> sending write buffer(0x03) seems spec violation (JESD220 Commands for
> exceptional behavior on UAC, SAM 5r05) before UAC clear (sd_probe).
> Anyway hpb reset query(ufshpb_check_hpb_reset_query) seems
> sufficient.
> Why do we need to send write buffer?
> 
> void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device
> *sdev)
> {
> out:
>     /* All LUs are initialized */
>     if (atomic_dec_and_test(&hba->ufshpb_dev.slave_conf_cnt))
> 	There seems to be a problem with this logic.
> 	If hpb is set on the last LUN, write buffer command is sent
> before sd_probe completes.
>         ufshpb_hpb_lu_prepared(hba);
> }
> 
> static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba)
> {
> 
>     init_success = !ufshpb_check_hpb_reset_query(hba);
> 
>     shost_for_each_device(sdev, hba->host) {
>         hpb = ufshpb_get_hpb_data(sdev);
>         if (!hpb)
>             continue;
> 
>         if (init_success) {
>             ufshpb_set_state(hpb, HPB_PRESENT);
>             if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0)
>                 queue_work(ufshpb_wq, &hpb->map_work);
>             if (!hpb->is_hcm)
>                 ufshpb_issue_umap_all_req(hpb); 
> 		This seems unnecessary.
> 		
>         } else {
> 
> Thanks 
> yohan

RE: Please check ufshpb init flow

[정요한(JOUNG YOHAN) Mobile SE]

> it's for "Inactivating all HPB regions" after reboot
> 
> scsi_add_lun()...>ufshpb_issue_umap_all_req(hpb);
> 
> if it is a cold reboot on both host side and UFS device side, it is not
> necessary to issue this write buffer.
According to you, is ufshpb_issue_umap_all_req used only for host reset?
During boottime, the problem is that the hpb is set to the last LUN, write buffer command is sent before sd_probe completes.
so, instead of performing unmap, UAC is returned. 
If ufshpb_issue_umap_all_req is not needed in cold boot, it seems necessary to move it to an appropriate location or remove
> 
> 
> On Fri, 2021-11-19 at 02:31 +0000, 정요한(JOUNG YOHAN) Mobile SE wrote:
> > Hi daejun
> >
> > Please check ufshpb init flow.
> > sending write buffer(0x03) seems spec violation (JESD220 Commands for
> > exceptional behavior on UAC, SAM 5r05) before UAC clear (sd_probe).
> > Anyway hpb reset query(ufshpb_check_hpb_reset_query) seems sufficient.
> > Why do we need to send write buffer?
> >
> > void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device
> > *sdev)
> > {
> > out:
> >     /* All LUs are initialized */
> >     if (atomic_dec_and_test(&hba->ufshpb_dev.slave_conf_cnt))
> > 	There seems to be a problem with this logic.
> > 	If hpb is set on the last LUN, write buffer command is sent before
> > sd_probe completes.
> >         ufshpb_hpb_lu_prepared(hba);
> > }
> >
> > static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba) {
> >
> >     init_success = !ufshpb_check_hpb_reset_query(hba);
> >
> >     shost_for_each_device(sdev, hba->host) {
> >         hpb = ufshpb_get_hpb_data(sdev);
> >         if (!hpb)
> >             continue;
> >
> >         if (init_success) {
> >             ufshpb_set_state(hpb, HPB_PRESENT);
> >             if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0)
> >                 queue_work(ufshpb_wq, &hpb->map_work);
> >             if (!hpb->is_hcm)
> >                 ufshpb_issue_umap_all_req(hpb);
> > 		This seems unnecessary.
> >
> >         } else {
> >
> > Thanks
> > yohan

Re: Please check ufshpb init flow

[Po-Wen Kao]

Hi Yohan and Bean,

I have propose a patch to remove ufshpb_issue_umap_all_req as we also
consider this a redundant operation.

Please refer to topic "[PATCH 1/1] scsi: ufs: remove redundant HPB
unmap". Feel free to leave comments there. Thanks~

Po-Wen
 
On Tue, 2021-11-23 at 05:19 +0000, 정요한(JOUNG YOHAN) Mobile SE wrote:
> > it's for "Inactivating all HPB regions" after reboot
> > 
> > scsi_add_lun()...>ufshpb_issue_umap_all_req(hpb);
> > 
> > if it is a cold reboot on both host side and UFS device side, it is
> > not
> > necessary to issue this write buffer.
> 
> According to you, is ufshpb_issue_umap_all_req used only for host
> reset?
> During boottime, the problem is that the hpb is set to the last LUN,
> write buffer command is sent before sd_probe completes.
> so, instead of performing unmap, UAC is returned. 
> If ufshpb_issue_umap_all_req is not needed in cold boot, it seems
> necessary to move it to an appropriate location or remove
> > 
> > 
> > On Fri, 2021-11-19 at 02:31 +0000, 정요한(JOUNG YOHAN) Mobile SE
> > wrote:
> > > Hi daejun
> > > 
> > > Please check ufshpb init flow.
> > > sending write buffer(0x03) seems spec violation (JESD220 Commands
> > > for
> > > exceptional behavior on UAC, SAM 5r05) before UAC clear
> > > (sd_probe).
> > > Anyway hpb reset query(ufshpb_check_hpb_reset_query) seems
> > > sufficient.
> > > Why do we need to send write buffer?
> > > 
> > > void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device
> > > *sdev)
> > > {
> > > out:
> > >     /* All LUs are initialized */
> > >     if (atomic_dec_and_test(&hba->ufshpb_dev.slave_conf_cnt))
> > > 	There seems to be a problem with this logic.
> > > 	If hpb is set on the last LUN, write buffer command is sent
> > > before
> > > sd_probe completes.
> > >         ufshpb_hpb_lu_prepared(hba);
> > > }
> > > 
> > > static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba) {
> > > 
> > >     init_success = !ufshpb_check_hpb_reset_query(hba);
> > > 
> > >     shost_for_each_device(sdev, hba->host) {
> > >         hpb = ufshpb_get_hpb_data(sdev);
> > >         if (!hpb)
> > >             continue;
> > > 
> > >         if (init_success) {
> > >             ufshpb_set_state(hpb, HPB_PRESENT);
> > >             if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0)
> > >                 queue_work(ufshpb_wq, &hpb->map_work);
> > >             if (!hpb->is_hcm)
> > >                 ufshpb_issue_umap_all_req(hpb);
> > > 		This seems unnecessary.
> > > 
> > >         } else {
> > > 
> > > Thanks
> > > yohan
> 
> 

Re: Please check ufshpb init flow

[Po-Wen Kao]

Hi Yohan and Bean,

I have propose a patch to remove ufshpb_issue_umap_all_req as we also
consider this a redundant operation.

Please refer to topic "[PATCH 1/1] scsi: ufs: remove redundant HPB
unmap". Feel free to leave comments there. 
Thanks~
Po-Wen
 
On Tue, 2021-11-23 at 05:19 +0000, 정요한(JOUNG YOHAN) Mobile SE wrote:
> > it's for "Inactivating all HPB regions" after reboot
> > 
> > scsi_add_lun()...>ufshpb_issue_umap_all_req(hpb);
> > 
> > if it is a cold reboot on both host side and UFS device side, it is
> > not
> > necessary to issue this write buffer.
> 
> According to you, is ufshpb_issue_umap_all_req used only for host
> reset?
> During boottime, the problem is that the hpb is set to the last LUN,
> write buffer command is sent before sd_probe completes.
> so, instead of performing unmap, UAC is returned. 
> If ufshpb_issue_umap_all_req is not needed in cold boot, it seems
> necessary to move it to an appropriate location or remove
> > 
> > 
> > On Fri, 2021-11-19 at 02:31 +0000, 정요한(JOUNG YOHAN) Mobile SE
> > wrote:
> > > Hi daejun
> > > 
> > > Please check ufshpb init flow.
> > > sending write buffer(0x03) seems spec violation (JESD220 Commands
> > > for
> > > exceptional behavior on UAC, SAM 5r05) before UAC clear
> > > (sd_probe).
> > > Anyway hpb reset query(ufshpb_check_hpb_reset_query) seems
> > > sufficient.
> > > Why do we need to send write buffer?
> > > 
> > > void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device
> > > *sdev)
> > > {
> > > out:
> > >     /* All LUs are initialized */
> > >     if (atomic_dec_and_test(&hba->ufshpb_dev.slave_conf_cnt))
> > > 	There seems to be a problem with this logic.
> > > 	If hpb is set on the last LUN, write buffer command is sent
> > > before
> > > sd_probe completes.
> > >         ufshpb_hpb_lu_prepared(hba);
> > > }
> > > 
> > > static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba) {
> > > 
> > >     init_success = !ufshpb_check_hpb_reset_query(hba);
> > > 
> > >     shost_for_each_device(sdev, hba->host) {
> > >         hpb = ufshpb_get_hpb_data(sdev);
> > >         if (!hpb)
> > >             continue;
> > > 
> > >         if (init_success) {
> > >             ufshpb_set_state(hpb, HPB_PRESENT);
> > >             if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0)
> > >                 queue_work(ufshpb_wq, &hpb->map_work);
> > >             if (!hpb->is_hcm)
> > >                 ufshpb_issue_umap_all_req(hpb);
> > > 		This seems unnecessary.
> > > 
> > >         } else {
> > > 
> > > Thanks
> > > yohan
> 
>