| Message ID | 20210302132503.224670-7-avri.altman@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Host control mode to HPB | expand |
On 2021-03-02 21:24, Avri Altman wrote: > The spec does not define what is the host's recommended response when > the device send hpb dev reset response (oper 0x2). > > We will update all active hpb regions: mark them and do that on the > next > read. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/ufs/ufshpb.c | 47 ++++++++++++++++++++++++++++++++++++--- > drivers/scsi/ufs/ufshpb.h | 2 ++ > 2 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index 0744feb4d484..0034fa03fdc6 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct > ufshcd_lrb *lrbp) > if (rgn->reads == ACTIVATION_THRESHOLD) > activate = true; > spin_unlock_irqrestore(&rgn->rgn_lock, flags); > - if (activate) { > + if (activate || > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { > spin_lock_irqsave(&hpb->rsp_list_lock, flags); > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); > hpb->stats.rb_active_cnt++; > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, > struct ufshcd_lrb *lrbp) > case HPB_RSP_DEV_RESET: > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, > "UFS device lost HPB information during PM.\n"); > + > + if (hpb->is_hcm) { > + struct scsi_device *sdev; bool need_reset = false; > + > + __shost_for_each_device(sdev, hba->host) { > + struct ufshpb_lu *h = sdev->hostdata; > + > + if (!h) > + continue; > + > + need_reset = true; > + } if (need_reset) schedule_work(&hpb->ufshpb_lun_reset_work); At last, scheduling only one reset work shall be enough, otherwise multiple reset work can be flying in parallel, so maybe above changes? > + } > + > break; > default: > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, > @@ -1594,6 +1609,25 @@ static void > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); > } > > +static void ufshpb_reset_work_handler(struct work_struct *work) > +{ > + struct ufshpb_lu *hpb; struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work); > + struct victim_select_info *lru_info; struct victim_select_info *lru_info = &hpb->lru_info; This can save some lines. Thanks, Can Guo. > + struct ufshpb_region *rgn; > + unsigned long flags; > + > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > + > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); > + > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > +} > + > static void ufshpb_normalization_work_handler(struct work_struct > *work) > { > struct ufshpb_lu *hpb; > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct > ufs_hba *hba, struct ufshpb_lu *hpb) > } else { > rgn->rgn_state = HPB_RGN_INACTIVE; > } > + > + rgn->rgn_flags = 0; > } > > return 0; > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba > *hba, struct ufshpb_lu *hpb) > INIT_LIST_HEAD(&hpb->list_hpb_lu); > > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); > - if (hpb->is_hcm) > + if (hpb->is_hcm) { > INIT_WORK(&hpb->ufshpb_normalization_work, > ufshpb_normalization_work_handler); > + INIT_WORK(&hpb->ufshpb_lun_reset_work, > + ufshpb_reset_work_handler); > + } > > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", > sizeof(struct ufshpb_req), 0, 0, NULL); > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct > ufshpb_lu *hpb) > > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) > { > - if (hpb->is_hcm) > + if (hpb->is_hcm) { > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); > cancel_work_sync(&hpb->ufshpb_normalization_work); > + } > cancel_work_sync(&hpb->map_work); > } > > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > index 84598a317897..37c1b0ea0c0a 100644 > --- a/drivers/scsi/ufs/ufshpb.h > +++ b/drivers/scsi/ufs/ufshpb.h > @@ -121,6 +121,7 @@ struct ufshpb_region { > struct list_head list_lru_rgn; > unsigned long rgn_flags; > #define RGN_FLAG_DIRTY 0 > +#define RGN_FLAG_UPDATE 1 > > /* region reads - for host mode */ > spinlock_t rgn_lock; > @@ -217,6 +218,7 @@ struct ufshpb_lu { > /* for selecting victim */ > struct victim_select_info lru_info; > struct work_struct ufshpb_normalization_work; > + struct work_struct ufshpb_lun_reset_work; > > /* pinned region information */ > u32 lu_pinned_start;
On 2021-03-15 09:34, Can Guo wrote: > On 2021-03-02 21:24, Avri Altman wrote: >> The spec does not define what is the host's recommended response when >> the device send hpb dev reset response (oper 0x2). >> >> We will update all active hpb regions: mark them and do that on the >> next >> read. >> >> Signed-off-by: Avri Altman <avri.altman@wdc.com> >> --- >> drivers/scsi/ufs/ufshpb.c | 47 >> ++++++++++++++++++++++++++++++++++++--- >> drivers/scsi/ufs/ufshpb.h | 2 ++ >> 2 files changed, 46 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >> index 0744feb4d484..0034fa03fdc6 100644 >> --- a/drivers/scsi/ufs/ufshpb.c >> +++ b/drivers/scsi/ufs/ufshpb.c >> @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct >> ufshcd_lrb *lrbp) >> if (rgn->reads == ACTIVATION_THRESHOLD) >> activate = true; >> spin_unlock_irqrestore(&rgn->rgn_lock, flags); >> - if (activate) { >> + if (activate || >> + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { >> spin_lock_irqsave(&hpb->rsp_list_lock, flags); >> ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); >> hpb->stats.rb_active_cnt++; >> @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, >> struct ufshcd_lrb *lrbp) >> case HPB_RSP_DEV_RESET: >> dev_warn(&hpb->sdev_ufs_lu->sdev_dev, >> "UFS device lost HPB information during PM.\n"); >> + >> + if (hpb->is_hcm) { >> + struct scsi_device *sdev; > bool need_reset = false; >> + >> + __shost_for_each_device(sdev, hba->host) { >> + struct ufshpb_lu *h = sdev->hostdata; >> + >> + if (!h) >> + continue; >> + >> + need_reset = true; >> + } > > if (need_reset) > schedule_work(&hpb->ufshpb_lun_reset_work); > > At last, scheduling only one reset work shall be enough, otherwise > multiple > reset work can be flying in parallel, so maybe above changes? Forget about this one, I misunderstood it - reset work is for each ufshpb_lu... Regards, Can Guo. > >> + } >> + >> break; >> default: >> dev_notice(&hpb->sdev_ufs_lu->sdev_dev, >> @@ -1594,6 +1609,25 @@ static void >> ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) >> spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); >> } >> >> +static void ufshpb_reset_work_handler(struct work_struct *work) >> +{ >> + struct ufshpb_lu *hpb; > > struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu, > ufshpb_lun_reset_work); > >> + struct victim_select_info *lru_info; > > struct victim_select_info *lru_info = &hpb->lru_info; > > This can save some lines. > > Thanks, > Can Guo. > >> + struct ufshpb_region *rgn; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&hpb->rgn_state_lock, flags); >> + >> + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) >> + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); >> + >> + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> +} >> + >> static void ufshpb_normalization_work_handler(struct work_struct >> *work) >> { >> struct ufshpb_lu *hpb; >> @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct >> ufs_hba *hba, struct ufshpb_lu *hpb) >> } else { >> rgn->rgn_state = HPB_RGN_INACTIVE; >> } >> + >> + rgn->rgn_flags = 0; >> } >> >> return 0; >> @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba >> *hba, struct ufshpb_lu *hpb) >> INIT_LIST_HEAD(&hpb->list_hpb_lu); >> >> INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); >> - if (hpb->is_hcm) >> + if (hpb->is_hcm) { >> INIT_WORK(&hpb->ufshpb_normalization_work, >> ufshpb_normalization_work_handler); >> + INIT_WORK(&hpb->ufshpb_lun_reset_work, >> + ufshpb_reset_work_handler); >> + } >> >> hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", >> sizeof(struct ufshpb_req), 0, 0, NULL); >> @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct >> ufshpb_lu *hpb) >> >> static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) >> { >> - if (hpb->is_hcm) >> + if (hpb->is_hcm) { >> + cancel_work_sync(&hpb->ufshpb_lun_reset_work); >> cancel_work_sync(&hpb->ufshpb_normalization_work); >> + } >> cancel_work_sync(&hpb->map_work); >> } >> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> index 84598a317897..37c1b0ea0c0a 100644 >> --- a/drivers/scsi/ufs/ufshpb.h >> +++ b/drivers/scsi/ufs/ufshpb.h >> @@ -121,6 +121,7 @@ struct ufshpb_region { >> struct list_head list_lru_rgn; >> unsigned long rgn_flags; >> #define RGN_FLAG_DIRTY 0 >> +#define RGN_FLAG_UPDATE 1 >> >> /* region reads - for host mode */ >> spinlock_t rgn_lock; >> @@ -217,6 +218,7 @@ struct ufshpb_lu { >> /* for selecting victim */ >> struct victim_select_info lru_info; >> struct work_struct ufshpb_normalization_work; >> + struct work_struct ufshpb_lun_reset_work; >> >> /* pinned region information */ >> u32 lu_pinned_start;
> > +static void ufshpb_reset_work_handler(struct work_struct *work) > > +{ > > + struct ufshpb_lu *hpb; > > struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu, > ufshpb_lun_reset_work); > > > + struct victim_select_info *lru_info; > > struct victim_select_info *lru_info = &hpb->lru_info; > > This can save some lines. Done. Thanks, Avri > > Thanks, > Can Guo. >
On 2021-03-02 21:24, Avri Altman wrote: > The spec does not define what is the host's recommended response when > the device send hpb dev reset response (oper 0x2). > > We will update all active hpb regions: mark them and do that on the > next > read. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/ufs/ufshpb.c | 47 ++++++++++++++++++++++++++++++++++++--- > drivers/scsi/ufs/ufshpb.h | 2 ++ > 2 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index 0744feb4d484..0034fa03fdc6 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct > ufshcd_lrb *lrbp) > if (rgn->reads == ACTIVATION_THRESHOLD) > activate = true; > spin_unlock_irqrestore(&rgn->rgn_lock, flags); > - if (activate) { > + if (activate || > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { > spin_lock_irqsave(&hpb->rsp_list_lock, flags); > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); > hpb->stats.rb_active_cnt++; > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, > struct ufshcd_lrb *lrbp) > case HPB_RSP_DEV_RESET: > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, > "UFS device lost HPB information during PM.\n"); > + > + if (hpb->is_hcm) { > + struct scsi_device *sdev; > + > + __shost_for_each_device(sdev, hba->host) { > + struct ufshpb_lu *h = sdev->hostdata; > + > + if (!h) > + continue; > + > + schedule_work(&hpb->ufshpb_lun_reset_work); > + } > + } > + > break; > default: > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, > @@ -1594,6 +1609,25 @@ static void > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); > } > > +static void ufshpb_reset_work_handler(struct work_struct *work) Just curious, directly doing below things inside ufshpb_rsp_upiu() does not seem a problem to me, does this really deserve a separate work? Thanks, Can Guo. > +{ > + struct ufshpb_lu *hpb; > + struct victim_select_info *lru_info; > + struct ufshpb_region *rgn; > + unsigned long flags; > + > + hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work); > + > + lru_info = &hpb->lru_info; > + > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > + > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); > + > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > +} > + > static void ufshpb_normalization_work_handler(struct work_struct > *work) > { > struct ufshpb_lu *hpb; > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct > ufs_hba *hba, struct ufshpb_lu *hpb) > } else { > rgn->rgn_state = HPB_RGN_INACTIVE; > } > + > + rgn->rgn_flags = 0; > } > > return 0; > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba > *hba, struct ufshpb_lu *hpb) > INIT_LIST_HEAD(&hpb->list_hpb_lu); > > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); > - if (hpb->is_hcm) > + if (hpb->is_hcm) { > INIT_WORK(&hpb->ufshpb_normalization_work, > ufshpb_normalization_work_handler); > + INIT_WORK(&hpb->ufshpb_lun_reset_work, > + ufshpb_reset_work_handler); > + } > > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", > sizeof(struct ufshpb_req), 0, 0, NULL); > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct > ufshpb_lu *hpb) > > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) > { > - if (hpb->is_hcm) > + if (hpb->is_hcm) { > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); > cancel_work_sync(&hpb->ufshpb_normalization_work); > + } > cancel_work_sync(&hpb->map_work); > } > > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > index 84598a317897..37c1b0ea0c0a 100644 > --- a/drivers/scsi/ufs/ufshpb.h > +++ b/drivers/scsi/ufs/ufshpb.h > @@ -121,6 +121,7 @@ struct ufshpb_region { > struct list_head list_lru_rgn; > unsigned long rgn_flags; > #define RGN_FLAG_DIRTY 0 > +#define RGN_FLAG_UPDATE 1 > > /* region reads - for host mode */ > spinlock_t rgn_lock; > @@ -217,6 +218,7 @@ struct ufshpb_lu { > /* for selecting victim */ > struct victim_select_info lru_info; > struct work_struct ufshpb_normalization_work; > + struct work_struct ufshpb_lun_reset_work; > > /* pinned region information */ > u32 lu_pinned_start;
> > On 2021-03-02 21:24, Avri Altman wrote: > > The spec does not define what is the host's recommended response when > > the device send hpb dev reset response (oper 0x2). > > > > We will update all active hpb regions: mark them and do that on the > > next > > read. > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > --- > > drivers/scsi/ufs/ufshpb.c | 47 > ++++++++++++++++++++++++++++++++++++--- > > drivers/scsi/ufs/ufshpb.h | 2 ++ > > 2 files changed, 46 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > > index 0744feb4d484..0034fa03fdc6 100644 > > --- a/drivers/scsi/ufs/ufshpb.c > > +++ b/drivers/scsi/ufs/ufshpb.c > > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct > > ufshcd_lrb *lrbp) > > if (rgn->reads == ACTIVATION_THRESHOLD) > > activate = true; > > spin_unlock_irqrestore(&rgn->rgn_lock, flags); > > - if (activate) { > > + if (activate || > > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { > > spin_lock_irqsave(&hpb->rsp_list_lock, flags); > > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); > > hpb->stats.rb_active_cnt++; > > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, > > struct ufshcd_lrb *lrbp) > > case HPB_RSP_DEV_RESET: > > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, > > "UFS device lost HPB information during PM.\n"); > > + > > + if (hpb->is_hcm) { > > + struct scsi_device *sdev; > > + > > + __shost_for_each_device(sdev, hba->host) { > > + struct ufshpb_lu *h = sdev->hostdata; > > + > > + if (!h) > > + continue; > > + > > + schedule_work(&hpb->ufshpb_lun_reset_work); > > + } > > + } > > + > > break; > > default: > > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, > > @@ -1594,6 +1609,25 @@ static void > > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) > > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); > > } > > > > +static void ufshpb_reset_work_handler(struct work_struct *work) > > Just curious, directly doing below things inside ufshpb_rsp_upiu() does > not > seem a problem to me, does this really deserve a separate work? I don't know, I never even consider of doing this. The active region list may contain up to few thousands of regions - It is not rare to see configurations that covers the entire device. But yes, I can do that. Better to get ack from Daejun first. Thanks, Avri > > Thanks, > Can Guo. > > > +{ > > + struct ufshpb_lu *hpb; > > + struct victim_select_info *lru_info; > > + struct ufshpb_region *rgn; > > + unsigned long flags; > > + > > + hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work); > > + > > + lru_info = &hpb->lru_info; > > + > > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > > + > > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) > > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); > > + > > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > > +} > > + > > static void ufshpb_normalization_work_handler(struct work_struct > > *work) > > { > > struct ufshpb_lu *hpb; > > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct > > ufs_hba *hba, struct ufshpb_lu *hpb) > > } else { > > rgn->rgn_state = HPB_RGN_INACTIVE; > > } > > + > > + rgn->rgn_flags = 0; > > } > > > > return 0; > > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba > > *hba, struct ufshpb_lu *hpb) > > INIT_LIST_HEAD(&hpb->list_hpb_lu); > > > > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); > > - if (hpb->is_hcm) > > + if (hpb->is_hcm) { > > INIT_WORK(&hpb->ufshpb_normalization_work, > > ufshpb_normalization_work_handler); > > + INIT_WORK(&hpb->ufshpb_lun_reset_work, > > + ufshpb_reset_work_handler); > > + } > > > > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", > > sizeof(struct ufshpb_req), 0, 0, NULL); > > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct > > ufshpb_lu *hpb) > > > > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) > > { > > - if (hpb->is_hcm) > > + if (hpb->is_hcm) { > > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); > > cancel_work_sync(&hpb->ufshpb_normalization_work); > > + } > > cancel_work_sync(&hpb->map_work); > > } > > > > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > > index 84598a317897..37c1b0ea0c0a 100644 > > --- a/drivers/scsi/ufs/ufshpb.h > > +++ b/drivers/scsi/ufs/ufshpb.h > > @@ -121,6 +121,7 @@ struct ufshpb_region { > > struct list_head list_lru_rgn; > > unsigned long rgn_flags; > > #define RGN_FLAG_DIRTY 0 > > +#define RGN_FLAG_UPDATE 1 > > > > /* region reads - for host mode */ > > spinlock_t rgn_lock; > > @@ -217,6 +218,7 @@ struct ufshpb_lu { > > /* for selecting victim */ > > struct victim_select_info lru_info; > > struct work_struct ufshpb_normalization_work; > > + struct work_struct ufshpb_lun_reset_work; > > > > /* pinned region information */ > > u32 lu_pinned_start;
On 2021-03-17 19:23, Avri Altman wrote: >> >> On 2021-03-02 21:24, Avri Altman wrote: >> > The spec does not define what is the host's recommended response when >> > the device send hpb dev reset response (oper 0x2). >> > >> > We will update all active hpb regions: mark them and do that on the >> > next >> > read. >> > >> > Signed-off-by: Avri Altman <avri.altman@wdc.com> >> > --- >> > drivers/scsi/ufs/ufshpb.c | 47 >> ++++++++++++++++++++++++++++++++++++--- >> > drivers/scsi/ufs/ufshpb.h | 2 ++ >> > 2 files changed, 46 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >> > index 0744feb4d484..0034fa03fdc6 100644 >> > --- a/drivers/scsi/ufs/ufshpb.c >> > +++ b/drivers/scsi/ufs/ufshpb.c >> > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct >> > ufshcd_lrb *lrbp) >> > if (rgn->reads == ACTIVATION_THRESHOLD) >> > activate = true; >> > spin_unlock_irqrestore(&rgn->rgn_lock, flags); >> > - if (activate) { >> > + if (activate || >> > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { >> > spin_lock_irqsave(&hpb->rsp_list_lock, flags); >> > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); >> > hpb->stats.rb_active_cnt++; >> > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, >> > struct ufshcd_lrb *lrbp) >> > case HPB_RSP_DEV_RESET: >> > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, >> > "UFS device lost HPB information during PM.\n"); >> > + >> > + if (hpb->is_hcm) { >> > + struct scsi_device *sdev; >> > + >> > + __shost_for_each_device(sdev, hba->host) { >> > + struct ufshpb_lu *h = sdev->hostdata; >> > + >> > + if (!h) >> > + continue; >> > + >> > + schedule_work(&hpb->ufshpb_lun_reset_work); >> > + } >> > + } >> > + >> > break; >> > default: >> > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, >> > @@ -1594,6 +1609,25 @@ static void >> > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) >> > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); >> > } >> > >> > +static void ufshpb_reset_work_handler(struct work_struct *work) >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu() >> does >> not >> seem a problem to me, does this really deserve a separate work? > I don't know, I never even consider of doing this. > The active region list may contain up to few thousands of regions - > It is not rare to see configurations that covers the entire device. > Yes, true, it can be a huge list. But what does the ops "HPB_RSP_DEV_RESET" really mean? The specs says "Device reset HPB Regions information", but I don't know what is really happening. Could you please elaborate? Thanks, Can Guo. > But yes, I can do that. > Better to get ack from Daejun first. > > Thanks, > Avri > >> >> Thanks, >> Can Guo. >> >> > +{ >> > + struct ufshpb_lu *hpb; >> > + struct victim_select_info *lru_info; >> > + struct ufshpb_region *rgn; >> > + unsigned long flags; >> > + >> > + hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work); >> > + >> > + lru_info = &hpb->lru_info; >> > + >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); >> > + >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); >> > + >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> > +} >> > + >> > static void ufshpb_normalization_work_handler(struct work_struct >> > *work) >> > { >> > struct ufshpb_lu *hpb; >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct >> > ufs_hba *hba, struct ufshpb_lu *hpb) >> > } else { >> > rgn->rgn_state = HPB_RGN_INACTIVE; >> > } >> > + >> > + rgn->rgn_flags = 0; >> > } >> > >> > return 0; >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba >> > *hba, struct ufshpb_lu *hpb) >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); >> > >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); >> > - if (hpb->is_hcm) >> > + if (hpb->is_hcm) { >> > INIT_WORK(&hpb->ufshpb_normalization_work, >> > ufshpb_normalization_work_handler); >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, >> > + ufshpb_reset_work_handler); >> > + } >> > >> > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", >> > sizeof(struct ufshpb_req), 0, 0, NULL); >> > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct >> > ufshpb_lu *hpb) >> > >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) >> > { >> > - if (hpb->is_hcm) >> > + if (hpb->is_hcm) { >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); >> > cancel_work_sync(&hpb->ufshpb_normalization_work); >> > + } >> > cancel_work_sync(&hpb->map_work); >> > } >> > >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> > index 84598a317897..37c1b0ea0c0a 100644 >> > --- a/drivers/scsi/ufs/ufshpb.h >> > +++ b/drivers/scsi/ufs/ufshpb.h >> > @@ -121,6 +121,7 @@ struct ufshpb_region { >> > struct list_head list_lru_rgn; >> > unsigned long rgn_flags; >> > #define RGN_FLAG_DIRTY 0 >> > +#define RGN_FLAG_UPDATE 1 >> > >> > /* region reads - for host mode */ >> > spinlock_t rgn_lock; >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { >> > /* for selecting victim */ >> > struct victim_select_info lru_info; >> > struct work_struct ufshpb_normalization_work; >> > + struct work_struct ufshpb_lun_reset_work; >> > >> > /* pinned region information */ >> > u32 lu_pinned_start;
> > On 2021-03-17 19:23, Avri Altman wrote: > >> > >> On 2021-03-02 21:24, Avri Altman wrote: > >> > The spec does not define what is the host's recommended response when > >> > the device send hpb dev reset response (oper 0x2). > >> > > >> > We will update all active hpb regions: mark them and do that on the > >> > next > >> > read. > >> > > >> > Signed-off-by: Avri Altman <avri.altman@wdc.com> > >> > --- > >> > drivers/scsi/ufs/ufshpb.c | 47 > >> ++++++++++++++++++++++++++++++++++++--- > >> > drivers/scsi/ufs/ufshpb.h | 2 ++ > >> > 2 files changed, 46 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > >> > index 0744feb4d484..0034fa03fdc6 100644 > >> > --- a/drivers/scsi/ufs/ufshpb.c > >> > +++ b/drivers/scsi/ufs/ufshpb.c > >> > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct > >> > ufshcd_lrb *lrbp) > >> > if (rgn->reads == ACTIVATION_THRESHOLD) > >> > activate = true; > >> > spin_unlock_irqrestore(&rgn->rgn_lock, flags); > >> > - if (activate) { > >> > + if (activate || > >> > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { > >> > spin_lock_irqsave(&hpb->rsp_list_lock, flags); > >> > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); > >> > hpb->stats.rb_active_cnt++; > >> > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, > >> > struct ufshcd_lrb *lrbp) > >> > case HPB_RSP_DEV_RESET: > >> > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, > >> > "UFS device lost HPB information during PM.\n"); > >> > + > >> > + if (hpb->is_hcm) { > >> > + struct scsi_device *sdev; > >> > + > >> > + __shost_for_each_device(sdev, hba->host) { > >> > + struct ufshpb_lu *h = sdev->hostdata; > >> > + > >> > + if (!h) > >> > + continue; > >> > + > >> > + schedule_work(&hpb->ufshpb_lun_reset_work); > >> > + } > >> > + } > >> > + > >> > break; > >> > default: > >> > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, > >> > @@ -1594,6 +1609,25 @@ static void > >> > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) > >> > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); > >> > } > >> > > >> > +static void ufshpb_reset_work_handler(struct work_struct *work) > >> > >> Just curious, directly doing below things inside ufshpb_rsp_upiu() > >> does > >> not > >> seem a problem to me, does this really deserve a separate work? > > I don't know, I never even consider of doing this. > > The active region list may contain up to few thousands of regions - > > It is not rare to see configurations that covers the entire device. > > > > Yes, true, it can be a huge list. But what does the ops > "HPB_RSP_DEV_RESET" > really mean? The specs says "Device reset HPB Regions information", but > I > don't know what is really happening. Could you please elaborate? It means that the device informs the host that the L2P cache is no longer valid. The spec doesn't say what to do in that case. We thought that in host mode, it make sense to update all the active regions. I think I will go with your suggestion. Effectively, in host mode, since it is deactivating "cold" regions, the lru list is kept relatively small, and contains only "hot" regions. Thanks, Avri > > Thanks, > Can Guo. > > > But yes, I can do that. > > Better to get ack from Daejun first. > > > > Thanks, > > Avri > > > >> > >> Thanks, > >> Can Guo. > >> > >> > +{ > >> > + struct ufshpb_lu *hpb; > >> > + struct victim_select_info *lru_info; > >> > + struct ufshpb_region *rgn; > >> > + unsigned long flags; > >> > + > >> > + hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work); > >> > + > >> > + lru_info = &hpb->lru_info; > >> > + > >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > >> > + > >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) > >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); > >> > + > >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > >> > +} > >> > + > >> > static void ufshpb_normalization_work_handler(struct work_struct > >> > *work) > >> > { > >> > struct ufshpb_lu *hpb; > >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct > >> > ufs_hba *hba, struct ufshpb_lu *hpb) > >> > } else { > >> > rgn->rgn_state = HPB_RGN_INACTIVE; > >> > } > >> > + > >> > + rgn->rgn_flags = 0; > >> > } > >> > > >> > return 0; > >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba > >> > *hba, struct ufshpb_lu *hpb) > >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); > >> > > >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); > >> > - if (hpb->is_hcm) > >> > + if (hpb->is_hcm) { > >> > INIT_WORK(&hpb->ufshpb_normalization_work, > >> > ufshpb_normalization_work_handler); > >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, > >> > + ufshpb_reset_work_handler); > >> > + } > >> > > >> > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", > >> > sizeof(struct ufshpb_req), 0, 0, NULL); > >> > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct > >> > ufshpb_lu *hpb) > >> > > >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) > >> > { > >> > - if (hpb->is_hcm) > >> > + if (hpb->is_hcm) { > >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); > >> > cancel_work_sync(&hpb->ufshpb_normalization_work); > >> > + } > >> > cancel_work_sync(&hpb->map_work); > >> > } > >> > > >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > >> > index 84598a317897..37c1b0ea0c0a 100644 > >> > --- a/drivers/scsi/ufs/ufshpb.h > >> > +++ b/drivers/scsi/ufs/ufshpb.h > >> > @@ -121,6 +121,7 @@ struct ufshpb_region { > >> > struct list_head list_lru_rgn; > >> > unsigned long rgn_flags; > >> > #define RGN_FLAG_DIRTY 0 > >> > +#define RGN_FLAG_UPDATE 1 > >> > > >> > /* region reads - for host mode */ > >> > spinlock_t rgn_lock; > >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { > >> > /* for selecting victim */ > >> > struct victim_select_info lru_info; > >> > struct work_struct ufshpb_normalization_work; > >> > + struct work_struct ufshpb_lun_reset_work; > >> > > >> > /* pinned region information */ > >> > u32 lu_pinned_start;
On 2021-03-17 20:22, Avri Altman wrote: >> >> On 2021-03-17 19:23, Avri Altman wrote: >> >> >> >> On 2021-03-02 21:24, Avri Altman wrote: >> >> > The spec does not define what is the host's recommended response when >> >> > the device send hpb dev reset response (oper 0x2). >> >> > >> >> > We will update all active hpb regions: mark them and do that on the >> >> > next >> >> > read. >> >> > >> >> > Signed-off-by: Avri Altman <avri.altman@wdc.com> >> >> > --- >> >> > drivers/scsi/ufs/ufshpb.c | 47 >> >> ++++++++++++++++++++++++++++++++++++--- >> >> > drivers/scsi/ufs/ufshpb.h | 2 ++ >> >> > 2 files changed, 46 insertions(+), 3 deletions(-) >> >> > >> >> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >> >> > index 0744feb4d484..0034fa03fdc6 100644 >> >> > --- a/drivers/scsi/ufs/ufshpb.c >> >> > +++ b/drivers/scsi/ufs/ufshpb.c >> >> > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct >> >> > ufshcd_lrb *lrbp) >> >> > if (rgn->reads == ACTIVATION_THRESHOLD) >> >> > activate = true; >> >> > spin_unlock_irqrestore(&rgn->rgn_lock, flags); >> >> > - if (activate) { >> >> > + if (activate || >> >> > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { Other than this place, do we also need to clear this bit in places like ufshpb_map_req_compl_fn() and/or ufshpb_cleanup_lru_info()? Otherwise, this flag may be left there even after the rgn is inactivated. >> >> > spin_lock_irqsave(&hpb->rsp_list_lock, flags); >> >> > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); >> >> > hpb->stats.rb_active_cnt++; >> >> > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, >> >> > struct ufshcd_lrb *lrbp) >> >> > case HPB_RSP_DEV_RESET: >> >> > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, >> >> > "UFS device lost HPB information during PM.\n"); >> >> > + >> >> > + if (hpb->is_hcm) { >> >> > + struct scsi_device *sdev; >> >> > + >> >> > + __shost_for_each_device(sdev, hba->host) { >> >> > + struct ufshpb_lu *h = sdev->hostdata; >> >> > + >> >> > + if (!h) >> >> > + continue; >> >> > + >> >> > + schedule_work(&hpb->ufshpb_lun_reset_work); >> >> > + } >> >> > + } >> >> > + >> >> > break; >> >> > default: >> >> > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, >> >> > @@ -1594,6 +1609,25 @@ static void >> >> > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) >> >> > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); >> >> > } >> >> > >> >> > +static void ufshpb_reset_work_handler(struct work_struct *work) >> >> >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu() >> >> does >> >> not >> >> seem a problem to me, does this really deserve a separate work? >> > I don't know, I never even consider of doing this. >> > The active region list may contain up to few thousands of regions - >> > It is not rare to see configurations that covers the entire device. >> > >> >> Yes, true, it can be a huge list. But what does the ops >> "HPB_RSP_DEV_RESET" >> really mean? The specs says "Device reset HPB Regions information", >> but >> I >> don't know what is really happening. Could you please elaborate? > It means that the device informs the host that the L2P cache is no > longer valid. > The spec doesn't say what to do in that case. Then it means that all the clean (without DIRTY flag set) HPB entries (ppns) in active rgns in host memory side may not be valid to the device anymore. Please correct me if I am wrong. > We thought that in host mode, it make sense to update all the active > regions. But current logic does not set the state of the sub-regions (in active regions) to INVALID, it only marks all active regions as UPDATE. Although one of subsequent read cmds shall put the sub-region back to activate_list, ufshpb_test_ppn_dirty() can still return false, thus these read cmds still think the ppns are valid and they shall move forward to send HPB Write Buffer (buffer id = 0x2, in case of HPB2.0) and HPB Read cmds. HPB Read cmds with invalid ppns will be treated as normal Read cmds by device as the specs says, but what would happen to HPB Write Buffer cmds (buffer id = 0x2, in case of HPB2.0) with invalid ppns? Can this be a real problem? > > I think I will go with your suggestion. > Effectively, in host mode, since it is deactivating "cold" regions, > the lru list is kept relatively small, and contains only "hot" regions. hmm... I don't really have a idea on this, please go with whatever you and Daejun think is fine here. Thanks, Can Guo. > > Thanks, > Avri > >> >> Thanks, >> Can Guo. >> >> > But yes, I can do that. >> > Better to get ack from Daejun first. >> > >> > Thanks, >> > Avri >> > >> >> >> >> Thanks, >> >> Can Guo. >> >> >> >> > +{ >> >> > + struct ufshpb_lu *hpb; >> >> > + struct victim_select_info *lru_info; >> >> > + struct ufshpb_region *rgn; >> >> > + unsigned long flags; >> >> > + >> >> > + hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work); >> >> > + >> >> > + lru_info = &hpb->lru_info; >> >> > + >> >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); >> >> > + >> >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) >> >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); >> >> > + >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> >> > +} >> >> > + >> >> > static void ufshpb_normalization_work_handler(struct work_struct >> >> > *work) >> >> > { >> >> > struct ufshpb_lu *hpb; >> >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct >> >> > ufs_hba *hba, struct ufshpb_lu *hpb) >> >> > } else { >> >> > rgn->rgn_state = HPB_RGN_INACTIVE; >> >> > } >> >> > + >> >> > + rgn->rgn_flags = 0; >> >> > } >> >> > >> >> > return 0; >> >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba >> >> > *hba, struct ufshpb_lu *hpb) >> >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); >> >> > >> >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); >> >> > - if (hpb->is_hcm) >> >> > + if (hpb->is_hcm) { >> >> > INIT_WORK(&hpb->ufshpb_normalization_work, >> >> > ufshpb_normalization_work_handler); >> >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, >> >> > + ufshpb_reset_work_handler); >> >> > + } >> >> > >> >> > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", >> >> > sizeof(struct ufshpb_req), 0, 0, NULL); >> >> > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct >> >> > ufshpb_lu *hpb) >> >> > >> >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) >> >> > { >> >> > - if (hpb->is_hcm) >> >> > + if (hpb->is_hcm) { >> >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); >> >> > cancel_work_sync(&hpb->ufshpb_normalization_work); >> >> > + } >> >> > cancel_work_sync(&hpb->map_work); >> >> > } >> >> > >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> >> > index 84598a317897..37c1b0ea0c0a 100644 >> >> > --- a/drivers/scsi/ufs/ufshpb.h >> >> > +++ b/drivers/scsi/ufs/ufshpb.h >> >> > @@ -121,6 +121,7 @@ struct ufshpb_region { >> >> > struct list_head list_lru_rgn; >> >> > unsigned long rgn_flags; >> >> > #define RGN_FLAG_DIRTY 0 >> >> > +#define RGN_FLAG_UPDATE 1 >> >> > >> >> > /* region reads - for host mode */ >> >> > spinlock_t rgn_lock; >> >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { >> >> > /* for selecting victim */ >> >> > struct victim_select_info lru_info; >> >> > struct work_struct ufshpb_normalization_work; >> >> > + struct work_struct ufshpb_lun_reset_work; >> >> > >> >> > /* pinned region information */ >> >> > u32 lu_pinned_start;
> > On 2021-03-17 20:22, Avri Altman wrote: > >> > >> On 2021-03-17 19:23, Avri Altman wrote: > >> >> > >> >> On 2021-03-02 21:24, Avri Altman wrote: > >> >> > The spec does not define what is the host's recommended response > when > >> >> > the device send hpb dev reset response (oper 0x2). > >> >> > > >> >> > We will update all active hpb regions: mark them and do that on the > >> >> > next > >> >> > read. > >> >> > > >> >> > Signed-off-by: Avri Altman <avri.altman@wdc.com> > >> >> > --- > >> >> > drivers/scsi/ufs/ufshpb.c | 47 > >> >> ++++++++++++++++++++++++++++++++++++--- > >> >> > drivers/scsi/ufs/ufshpb.h | 2 ++ > >> >> > 2 files changed, 46 insertions(+), 3 deletions(-) > >> >> > > >> >> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > >> >> > index 0744feb4d484..0034fa03fdc6 100644 > >> >> > --- a/drivers/scsi/ufs/ufshpb.c > >> >> > +++ b/drivers/scsi/ufs/ufshpb.c > >> >> > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct > >> >> > ufshcd_lrb *lrbp) > >> >> > if (rgn->reads == ACTIVATION_THRESHOLD) > >> >> > activate = true; > >> >> > spin_unlock_irqrestore(&rgn->rgn_lock, flags); > >> >> > - if (activate) { > >> >> > + if (activate || > >> >> > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { > > Other than this place, do we also need to clear this bit in places like > ufshpb_map_req_compl_fn() and/or ufshpb_cleanup_lru_info()? Otherwise, > this flag may be left there even after the rgn is inactivated. I don't think so - may cause a race if device reset arrives when map request just finished. Better to be in one place. > > >> >> > spin_lock_irqsave(&hpb->rsp_list_lock, flags); > >> >> > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); > >> >> > hpb->stats.rb_active_cnt++; > >> >> > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba > *hba, > >> >> > struct ufshcd_lrb *lrbp) > >> >> > case HPB_RSP_DEV_RESET: > >> >> > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, > >> >> > "UFS device lost HPB information during PM.\n"); > >> >> > + > >> >> > + if (hpb->is_hcm) { > >> >> > + struct scsi_device *sdev; > >> >> > + > >> >> > + __shost_for_each_device(sdev, hba->host) { > >> >> > + struct ufshpb_lu *h = sdev->hostdata; > >> >> > + > >> >> > + if (!h) > >> >> > + continue; > >> >> > + > >> >> > + schedule_work(&hpb->ufshpb_lun_reset_work); > >> >> > + } > >> >> > + } > >> >> > + > >> >> > break; > >> >> > default: > >> >> > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, > >> >> > @@ -1594,6 +1609,25 @@ static void > >> >> > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) > >> >> > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); > >> >> > } > >> >> > > >> >> > +static void ufshpb_reset_work_handler(struct work_struct *work) > >> >> > >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu() > >> >> does > >> >> not > >> >> seem a problem to me, does this really deserve a separate work? > >> > I don't know, I never even consider of doing this. > >> > The active region list may contain up to few thousands of regions - > >> > It is not rare to see configurations that covers the entire device. > >> > > >> > >> Yes, true, it can be a huge list. But what does the ops > >> "HPB_RSP_DEV_RESET" > >> really mean? The specs says "Device reset HPB Regions information", > >> but > >> I > >> don't know what is really happening. Could you please elaborate? > > It means that the device informs the host that the L2P cache is no > > longer valid. > > The spec doesn't say what to do in that case. > > Then it means that all the clean (without DIRTY flag set) HPB entries > (ppns) > in active rgns in host memory side may not be valid to the device > anymore. > Please correct me if I am wrong. > > > We thought that in host mode, it make sense to update all the active > > regions. > > But current logic does not set the state of the sub-regions (in active > regions) to > INVALID, it only marks all active regions as UPDATE. > > Although one of subsequent read cmds shall put the sub-region back to > activate_list, > ufshpb_test_ppn_dirty() can still return false, thus these read cmds > still think the > ppns are valid and they shall move forward to send HPB Write Buffer > (buffer id = 0x2, > in case of HPB2.0) and HPB Read cmds. > > HPB Read cmds with invalid ppns will be treated as normal Read cmds by > device as the > specs says, but what would happen to HPB Write Buffer cmds (buffer id = > 0x2, in case > of HPB2.0) with invalid ppns? Can this be a real problem? No need to control the ppn dirty / invalid state for this case. The device send device reset so it is aware that all the L2P cache is invalid. Any HPB_READ is treated like normal READ10. Only once HPB-READ-BUFFER is completed, the device will relate back to the physical address. > > > > > I think I will go with your suggestion. > > Effectively, in host mode, since it is deactivating "cold" regions, > > the lru list is kept relatively small, and contains only "hot" regions. > > hmm... I don't really have a idea on this, please go with whatever you > and Daejun think is fine here. I will take your advice and remove the worker. Thanks, Avri > > Thanks, > Can Guo. > > > > > Thanks, > > Avri > > > >> > >> Thanks, > >> Can Guo. > >> > >> > But yes, I can do that. > >> > Better to get ack from Daejun first. > >> > > >> > Thanks, > >> > Avri > >> > > >> >> > >> >> Thanks, > >> >> Can Guo. > >> >> > >> >> > +{ > >> >> > + struct ufshpb_lu *hpb; > >> >> > + struct victim_select_info *lru_info; > >> >> > + struct ufshpb_region *rgn; > >> >> > + unsigned long flags; > >> >> > + > >> >> > + hpb = container_of(work, struct ufshpb_lu, > ufshpb_lun_reset_work); > >> >> > + > >> >> > + lru_info = &hpb->lru_info; > >> >> > + > >> >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > >> >> > + > >> >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) > >> >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); > >> >> > + > >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > >> >> > +} > >> >> > + > >> >> > static void ufshpb_normalization_work_handler(struct work_struct > >> >> > *work) > >> >> > { > >> >> > struct ufshpb_lu *hpb; > >> >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct > >> >> > ufs_hba *hba, struct ufshpb_lu *hpb) > >> >> > } else { > >> >> > rgn->rgn_state = HPB_RGN_INACTIVE; > >> >> > } > >> >> > + > >> >> > + rgn->rgn_flags = 0; > >> >> > } > >> >> > > >> >> > return 0; > >> >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct > ufs_hba > >> >> > *hba, struct ufshpb_lu *hpb) > >> >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); > >> >> > > >> >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); > >> >> > - if (hpb->is_hcm) > >> >> > + if (hpb->is_hcm) { > >> >> > INIT_WORK(&hpb->ufshpb_normalization_work, > >> >> > ufshpb_normalization_work_handler); > >> >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, > >> >> > + ufshpb_reset_work_handler); > >> >> > + } > >> >> > > >> >> > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", > >> >> > sizeof(struct ufshpb_req), 0, 0, NULL); > >> >> > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct > >> >> > ufshpb_lu *hpb) > >> >> > > >> >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) > >> >> > { > >> >> > - if (hpb->is_hcm) > >> >> > + if (hpb->is_hcm) { > >> >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); > >> >> > cancel_work_sync(&hpb->ufshpb_normalization_work); > >> >> > + } > >> >> > cancel_work_sync(&hpb->map_work); > >> >> > } > >> >> > > >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > >> >> > index 84598a317897..37c1b0ea0c0a 100644 > >> >> > --- a/drivers/scsi/ufs/ufshpb.h > >> >> > +++ b/drivers/scsi/ufs/ufshpb.h > >> >> > @@ -121,6 +121,7 @@ struct ufshpb_region { > >> >> > struct list_head list_lru_rgn; > >> >> > unsigned long rgn_flags; > >> >> > #define RGN_FLAG_DIRTY 0 > >> >> > +#define RGN_FLAG_UPDATE 1 > >> >> > > >> >> > /* region reads - for host mode */ > >> >> > spinlock_t rgn_lock; > >> >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { > >> >> > /* for selecting victim */ > >> >> > struct victim_select_info lru_info; > >> >> > struct work_struct ufshpb_normalization_work; > >> >> > + struct work_struct ufshpb_lun_reset_work; > >> >> > > >> >> > /* pinned region information */ > >> >> > u32 lu_pinned_start;
On 2021-03-17 22:22, Avri Altman wrote: >> >> On 2021-03-17 20:22, Avri Altman wrote: >> >> >> >> On 2021-03-17 19:23, Avri Altman wrote: >> >> >> >> >> >> On 2021-03-02 21:24, Avri Altman wrote: >> >> >> > The spec does not define what is the host's recommended response >> when >> >> >> > the device send hpb dev reset response (oper 0x2). >> >> >> > >> >> >> > We will update all active hpb regions: mark them and do that on the >> >> >> > next >> >> >> > read. >> >> >> > >> >> >> > Signed-off-by: Avri Altman <avri.altman@wdc.com> >> >> >> > --- >> >> >> > drivers/scsi/ufs/ufshpb.c | 47 >> >> >> ++++++++++++++++++++++++++++++++++++--- >> >> >> > drivers/scsi/ufs/ufshpb.h | 2 ++ >> >> >> > 2 files changed, 46 insertions(+), 3 deletions(-) >> >> >> > >> >> >> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >> >> >> > index 0744feb4d484..0034fa03fdc6 100644 >> >> >> > --- a/drivers/scsi/ufs/ufshpb.c >> >> >> > +++ b/drivers/scsi/ufs/ufshpb.c >> >> >> > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct >> >> >> > ufshcd_lrb *lrbp) >> >> >> > if (rgn->reads == ACTIVATION_THRESHOLD) >> >> >> > activate = true; >> >> >> > spin_unlock_irqrestore(&rgn->rgn_lock, flags); >> >> >> > - if (activate) { >> >> >> > + if (activate || >> >> >> > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { >> >> Other than this place, do we also need to clear this bit in places >> like >> ufshpb_map_req_compl_fn() and/or ufshpb_cleanup_lru_info()? Otherwise, >> this flag may be left there even after the rgn is inactivated. > I don't think so - may cause a race if device reset arrives when map > request just finished. hmm.. that does not look racy to me, in that case the bit is either set or not set, which does not hurt anything. Anyways, it is up to you. > Better to be in one place. > >> >> >> >> > spin_lock_irqsave(&hpb->rsp_list_lock, flags); >> >> >> > ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); >> >> >> > hpb->stats.rb_active_cnt++; >> >> >> > @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba >> *hba, >> >> >> > struct ufshcd_lrb *lrbp) >> >> >> > case HPB_RSP_DEV_RESET: >> >> >> > dev_warn(&hpb->sdev_ufs_lu->sdev_dev, >> >> >> > "UFS device lost HPB information during PM.\n"); >> >> >> > + >> >> >> > + if (hpb->is_hcm) { >> >> >> > + struct scsi_device *sdev; >> >> >> > + >> >> >> > + __shost_for_each_device(sdev, hba->host) { >> >> >> > + struct ufshpb_lu *h = sdev->hostdata; >> >> >> > + >> >> >> > + if (!h) >> >> >> > + continue; >> >> >> > + >> >> >> > + schedule_work(&hpb->ufshpb_lun_reset_work); >> >> >> > + } >> >> >> > + } >> >> >> > + >> >> >> > break; >> >> >> > default: >> >> >> > dev_notice(&hpb->sdev_ufs_lu->sdev_dev, >> >> >> > @@ -1594,6 +1609,25 @@ static void >> >> >> > ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) >> >> >> > spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); >> >> >> > } >> >> >> > >> >> >> > +static void ufshpb_reset_work_handler(struct work_struct *work) >> >> >> >> >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu() >> >> >> does >> >> >> not >> >> >> seem a problem to me, does this really deserve a separate work? >> >> > I don't know, I never even consider of doing this. >> >> > The active region list may contain up to few thousands of regions - >> >> > It is not rare to see configurations that covers the entire device. >> >> > >> >> >> >> Yes, true, it can be a huge list. But what does the ops >> >> "HPB_RSP_DEV_RESET" >> >> really mean? The specs says "Device reset HPB Regions information", >> >> but >> >> I >> >> don't know what is really happening. Could you please elaborate? >> > It means that the device informs the host that the L2P cache is no >> > longer valid. >> > The spec doesn't say what to do in that case. >> >> Then it means that all the clean (without DIRTY flag set) HPB entries >> (ppns) >> in active rgns in host memory side may not be valid to the device >> anymore. >> Please correct me if I am wrong. >> >> > We thought that in host mode, it make sense to update all the active >> > regions. >> >> But current logic does not set the state of the sub-regions (in active >> regions) to >> INVALID, it only marks all active regions as UPDATE. >> >> Although one of subsequent read cmds shall put the sub-region back to >> activate_list, >> ufshpb_test_ppn_dirty() can still return false, thus these read cmds >> still think the >> ppns are valid and they shall move forward to send HPB Write Buffer >> (buffer id = 0x2, >> in case of HPB2.0) and HPB Read cmds. >> >> HPB Read cmds with invalid ppns will be treated as normal Read cmds by >> device as the >> specs says, but what would happen to HPB Write Buffer cmds (buffer id >> = >> 0x2, in case >> of HPB2.0) with invalid ppns? Can this be a real problem? > No need to control the ppn dirty / invalid state for this case. > The device send device reset so it is aware that all the L2P cache is > invalid. > Any HPB_READ is treated like normal READ10. > > Only once HPB-READ-BUFFER is completed, > the device will relate back to the physical address. What about HPB-WRITE-BUFFER (buffer id = 0x2) cmds? Thanks, Can Guo. > >> >> > >> > I think I will go with your suggestion. >> > Effectively, in host mode, since it is deactivating "cold" regions, >> > the lru list is kept relatively small, and contains only "hot" regions. >> >> hmm... I don't really have a idea on this, please go with whatever you >> and Daejun think is fine here. > I will take your advice and remove the worker. > > > Thanks, > Avri > >> >> Thanks, >> Can Guo. >> >> > >> > Thanks, >> > Avri >> > >> >> >> >> Thanks, >> >> Can Guo. >> >> >> >> > But yes, I can do that. >> >> > Better to get ack from Daejun first. >> >> > >> >> > Thanks, >> >> > Avri >> >> > >> >> >> >> >> >> Thanks, >> >> >> Can Guo. >> >> >> >> >> >> > +{ >> >> >> > + struct ufshpb_lu *hpb; >> >> >> > + struct victim_select_info *lru_info; >> >> >> > + struct ufshpb_region *rgn; >> >> >> > + unsigned long flags; >> >> >> > + >> >> >> > + hpb = container_of(work, struct ufshpb_lu, >> ufshpb_lun_reset_work); >> >> >> > + >> >> >> > + lru_info = &hpb->lru_info; >> >> >> > + >> >> >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); >> >> >> > + >> >> >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) >> >> >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); >> >> >> > + >> >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> >> >> > +} >> >> >> > + >> >> >> > static void ufshpb_normalization_work_handler(struct work_struct >> >> >> > *work) >> >> >> > { >> >> >> > struct ufshpb_lu *hpb; >> >> >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct >> >> >> > ufs_hba *hba, struct ufshpb_lu *hpb) >> >> >> > } else { >> >> >> > rgn->rgn_state = HPB_RGN_INACTIVE; >> >> >> > } >> >> >> > + >> >> >> > + rgn->rgn_flags = 0; >> >> >> > } >> >> >> > >> >> >> > return 0; >> >> >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct >> ufs_hba >> >> >> > *hba, struct ufshpb_lu *hpb) >> >> >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); >> >> >> > >> >> >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); >> >> >> > - if (hpb->is_hcm) >> >> >> > + if (hpb->is_hcm) { >> >> >> > INIT_WORK(&hpb->ufshpb_normalization_work, >> >> >> > ufshpb_normalization_work_handler); >> >> >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, >> >> >> > + ufshpb_reset_work_handler); >> >> >> > + } >> >> >> > >> >> >> > hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", >> >> >> > sizeof(struct ufshpb_req), 0, 0, NULL); >> >> >> > @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct >> >> >> > ufshpb_lu *hpb) >> >> >> > >> >> >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) >> >> >> > { >> >> >> > - if (hpb->is_hcm) >> >> >> > + if (hpb->is_hcm) { >> >> >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); >> >> >> > cancel_work_sync(&hpb->ufshpb_normalization_work); >> >> >> > + } >> >> >> > cancel_work_sync(&hpb->map_work); >> >> >> > } >> >> >> > >> >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> >> >> > index 84598a317897..37c1b0ea0c0a 100644 >> >> >> > --- a/drivers/scsi/ufs/ufshpb.h >> >> >> > +++ b/drivers/scsi/ufs/ufshpb.h >> >> >> > @@ -121,6 +121,7 @@ struct ufshpb_region { >> >> >> > struct list_head list_lru_rgn; >> >> >> > unsigned long rgn_flags; >> >> >> > #define RGN_FLAG_DIRTY 0 >> >> >> > +#define RGN_FLAG_UPDATE 1 >> >> >> > >> >> >> > /* region reads - for host mode */ >> >> >> > spinlock_t rgn_lock; >> >> >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { >> >> >> > /* for selecting victim */ >> >> >> > struct victim_select_info lru_info; >> >> >> > struct work_struct ufshpb_normalization_work; >> >> >> > + struct work_struct ufshpb_lun_reset_work; >> >> >> > >> >> >> > /* pinned region information */ >> >> >> > u32 lu_pinned_start;
> >> >> >> > >> >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu() > >> >> >> does > >> >> >> not > >> >> >> seem a problem to me, does this really deserve a separate work? > >> >> > I don't know, I never even consider of doing this. > >> >> > The active region list may contain up to few thousands of regions - > >> >> > It is not rare to see configurations that covers the entire device. > >> >> > > >> >> > >> >> Yes, true, it can be a huge list. But what does the ops > >> >> "HPB_RSP_DEV_RESET" > >> >> really mean? The specs says "Device reset HPB Regions information", > >> >> but > >> >> I > >> >> don't know what is really happening. Could you please elaborate? > >> > It means that the device informs the host that the L2P cache is no > >> > longer valid. > >> > The spec doesn't say what to do in that case. > >> > >> Then it means that all the clean (without DIRTY flag set) HPB entries > >> (ppns) > >> in active rgns in host memory side may not be valid to the device > >> anymore. > >> Please correct me if I am wrong. > >> > >> > We thought that in host mode, it make sense to update all the active > >> > regions. > >> > >> But current logic does not set the state of the sub-regions (in active > >> regions) to > >> INVALID, it only marks all active regions as UPDATE. > >> > >> Although one of subsequent read cmds shall put the sub-region back to > >> activate_list, > >> ufshpb_test_ppn_dirty() can still return false, thus these read cmds > >> still think the > >> ppns are valid and they shall move forward to send HPB Write Buffer > >> (buffer id = 0x2, > >> in case of HPB2.0) and HPB Read cmds. > >> > >> HPB Read cmds with invalid ppns will be treated as normal Read cmds by > >> device as the > >> specs says, but what would happen to HPB Write Buffer cmds (buffer id > >> = > >> 0x2, in case > >> of HPB2.0) with invalid ppns? Can this be a real problem? > > No need to control the ppn dirty / invalid state for this case. > > The device send device reset so it is aware that all the L2P cache is > > invalid. > > Any HPB_READ is treated like normal READ10. > > > > Only once HPB-READ-BUFFER is completed, > > the device will relate back to the physical address. > > What about HPB-WRITE-BUFFER (buffer id = 0x2) cmds? Same. Oper 0x2 is a relative simple case. The device is expected to manage some versioning framework not to be "fooled" by erroneous ppn. There are some more challenging races that the device should meet. Thanks, Avri > > Thanks, > Can Guo. > > > > >> > >> > > >> > I think I will go with your suggestion. > >> > Effectively, in host mode, since it is deactivating "cold" regions, > >> > the lru list is kept relatively small, and contains only "hot" regions. > >> > >> hmm... I don't really have a idea on this, please go with whatever you > >> and Daejun think is fine here. > > I will take your advice and remove the worker. > > > > > > Thanks, > > Avri > > > >> > >> Thanks, > >> Can Guo. > >> > >> > > >> > Thanks, > >> > Avri > >> > > >> >> > >> >> Thanks, > >> >> Can Guo. > >> >> > >> >> > But yes, I can do that. > >> >> > Better to get ack from Daejun first. > >> >> > > >> >> > Thanks, > >> >> > Avri > >> >> > > >> >> >> > >> >> >> Thanks, > >> >> >> Can Guo. > >> >> >> > >> >> >> > +{ > >> >> >> > + struct ufshpb_lu *hpb; > >> >> >> > + struct victim_select_info *lru_info; > >> >> >> > + struct ufshpb_region *rgn; > >> >> >> > + unsigned long flags; > >> >> >> > + > >> >> >> > + hpb = container_of(work, struct ufshpb_lu, > >> ufshpb_lun_reset_work); > >> >> >> > + > >> >> >> > + lru_info = &hpb->lru_info; > >> >> >> > + > >> >> >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > >> >> >> > + > >> >> >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) > >> >> >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); > >> >> >> > + > >> >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > >> >> >> > +} > >> >> >> > + > >> >> >> > static void ufshpb_normalization_work_handler(struct work_struct > >> >> >> > *work) > >> >> >> > { > >> >> >> > struct ufshpb_lu *hpb; > >> >> >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct > >> >> >> > ufs_hba *hba, struct ufshpb_lu *hpb) > >> >> >> > } else { > >> >> >> > rgn->rgn_state = HPB_RGN_INACTIVE; > >> >> >> > } > >> >> >> > + > >> >> >> > + rgn->rgn_flags = 0; > >> >> >> > } > >> >> >> > > >> >> >> > return 0; > >> >> >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct > >> ufs_hba > >> >> >> > *hba, struct ufshpb_lu *hpb) > >> >> >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); > >> >> >> > > >> >> >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); > >> >> >> > - if (hpb->is_hcm) > >> >> >> > + if (hpb->is_hcm) { > >> >> >> > INIT_WORK(&hpb->ufshpb_normalization_work, > >> >> >> > ufshpb_normalization_work_handler); > >> >> >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, > >> >> >> > + ufshpb_reset_work_handler); > >> >> >> > + } > >> >> >> > > >> >> >> > hpb->map_req_cache = > kmem_cache_create("ufshpb_req_cache", > >> >> >> > sizeof(struct ufshpb_req), 0, 0, NULL); > >> >> >> > @@ -2114,8 +2153,10 @@ static void > ufshpb_discard_rsp_lists(struct > >> >> >> > ufshpb_lu *hpb) > >> >> >> > > >> >> >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) > >> >> >> > { > >> >> >> > - if (hpb->is_hcm) > >> >> >> > + if (hpb->is_hcm) { > >> >> >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); > >> >> >> > cancel_work_sync(&hpb->ufshpb_normalization_work); > >> >> >> > + } > >> >> >> > cancel_work_sync(&hpb->map_work); > >> >> >> > } > >> >> >> > > >> >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > >> >> >> > index 84598a317897..37c1b0ea0c0a 100644 > >> >> >> > --- a/drivers/scsi/ufs/ufshpb.h > >> >> >> > +++ b/drivers/scsi/ufs/ufshpb.h > >> >> >> > @@ -121,6 +121,7 @@ struct ufshpb_region { > >> >> >> > struct list_head list_lru_rgn; > >> >> >> > unsigned long rgn_flags; > >> >> >> > #define RGN_FLAG_DIRTY 0 > >> >> >> > +#define RGN_FLAG_UPDATE 1 > >> >> >> > > >> >> >> > /* region reads - for host mode */ > >> >> >> > spinlock_t rgn_lock; > >> >> >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { > >> >> >> > /* for selecting victim */ > >> >> >> > struct victim_select_info lru_info; > >> >> >> > struct work_struct ufshpb_normalization_work; > >> >> >> > + struct work_struct ufshpb_lun_reset_work; > >> >> >> > > >> >> >> > /* pinned region information */ > >> >> >> > u32 lu_pinned_start;
On 2021-03-17 23:46, Avri Altman wrote: >> >> >> >> >> >> >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu() >> >> >> >> does >> >> >> >> not >> >> >> >> seem a problem to me, does this really deserve a separate work? >> >> >> > I don't know, I never even consider of doing this. >> >> >> > The active region list may contain up to few thousands of regions - >> >> >> > It is not rare to see configurations that covers the entire device. >> >> >> > >> >> >> >> >> >> Yes, true, it can be a huge list. But what does the ops >> >> >> "HPB_RSP_DEV_RESET" >> >> >> really mean? The specs says "Device reset HPB Regions information", >> >> >> but >> >> >> I >> >> >> don't know what is really happening. Could you please elaborate? >> >> > It means that the device informs the host that the L2P cache is no >> >> > longer valid. >> >> > The spec doesn't say what to do in that case. >> >> >> >> Then it means that all the clean (without DIRTY flag set) HPB entries >> >> (ppns) >> >> in active rgns in host memory side may not be valid to the device >> >> anymore. >> >> Please correct me if I am wrong. >> >> >> >> > We thought that in host mode, it make sense to update all the active >> >> > regions. >> >> >> >> But current logic does not set the state of the sub-regions (in active >> >> regions) to >> >> INVALID, it only marks all active regions as UPDATE. >> >> >> >> Although one of subsequent read cmds shall put the sub-region back to >> >> activate_list, >> >> ufshpb_test_ppn_dirty() can still return false, thus these read cmds >> >> still think the >> >> ppns are valid and they shall move forward to send HPB Write Buffer >> >> (buffer id = 0x2, >> >> in case of HPB2.0) and HPB Read cmds. >> >> >> >> HPB Read cmds with invalid ppns will be treated as normal Read cmds by >> >> device as the >> >> specs says, but what would happen to HPB Write Buffer cmds (buffer id >> >> = >> >> 0x2, in case >> >> of HPB2.0) with invalid ppns? Can this be a real problem? >> > No need to control the ppn dirty / invalid state for this case. >> > The device send device reset so it is aware that all the L2P cache is >> > invalid. >> > Any HPB_READ is treated like normal READ10. >> > >> > Only once HPB-READ-BUFFER is completed, >> > the device will relate back to the physical address. >> >> What about HPB-WRITE-BUFFER (buffer id = 0x2) cmds? > Same. > Oper 0x2 is a relative simple case. > The device is expected to manage some versioning framework not to be > "fooled" by erroneous ppn. > There are some more challenging races that the device should meet. > But I don't find the handling w.r.t this scenario on HPB2.0 specs - how would the device re-act/respond to HPB-WRITE-BUFFER cmds with invalid HPB entries? Could you please point me to relevant section/paragraph? Thanks, Can Guo. > Thanks, > Avri >> >> Thanks, >> Can Guo. >> >> > >> >> >> >> > >> >> > I think I will go with your suggestion. >> >> > Effectively, in host mode, since it is deactivating "cold" regions, >> >> > the lru list is kept relatively small, and contains only "hot" regions. >> >> >> >> hmm... I don't really have a idea on this, please go with whatever you >> >> and Daejun think is fine here. >> > I will take your advice and remove the worker. >> > >> > >> > Thanks, >> > Avri >> > >> >> >> >> Thanks, >> >> Can Guo. >> >> >> >> > >> >> > Thanks, >> >> > Avri >> >> > >> >> >> >> >> >> Thanks, >> >> >> Can Guo. >> >> >> >> >> >> > But yes, I can do that. >> >> >> > Better to get ack from Daejun first. >> >> >> > >> >> >> > Thanks, >> >> >> > Avri >> >> >> > >> >> >> >> >> >> >> >> Thanks, >> >> >> >> Can Guo. >> >> >> >> >> >> >> >> > +{ >> >> >> >> > + struct ufshpb_lu *hpb; >> >> >> >> > + struct victim_select_info *lru_info; >> >> >> >> > + struct ufshpb_region *rgn; >> >> >> >> > + unsigned long flags; >> >> >> >> > + >> >> >> >> > + hpb = container_of(work, struct ufshpb_lu, >> >> ufshpb_lun_reset_work); >> >> >> >> > + >> >> >> >> > + lru_info = &hpb->lru_info; >> >> >> >> > + >> >> >> >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); >> >> >> >> > + >> >> >> >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) >> >> >> >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); >> >> >> >> > + >> >> >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> >> >> >> > +} >> >> >> >> > + >> >> >> >> > static void ufshpb_normalization_work_handler(struct work_struct >> >> >> >> > *work) >> >> >> >> > { >> >> >> >> > struct ufshpb_lu *hpb; >> >> >> >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct >> >> >> >> > ufs_hba *hba, struct ufshpb_lu *hpb) >> >> >> >> > } else { >> >> >> >> > rgn->rgn_state = HPB_RGN_INACTIVE; >> >> >> >> > } >> >> >> >> > + >> >> >> >> > + rgn->rgn_flags = 0; >> >> >> >> > } >> >> >> >> > >> >> >> >> > return 0; >> >> >> >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct >> >> ufs_hba >> >> >> >> > *hba, struct ufshpb_lu *hpb) >> >> >> >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); >> >> >> >> > >> >> >> >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); >> >> >> >> > - if (hpb->is_hcm) >> >> >> >> > + if (hpb->is_hcm) { >> >> >> >> > INIT_WORK(&hpb->ufshpb_normalization_work, >> >> >> >> > ufshpb_normalization_work_handler); >> >> >> >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, >> >> >> >> > + ufshpb_reset_work_handler); >> >> >> >> > + } >> >> >> >> > >> >> >> >> > hpb->map_req_cache = >> kmem_cache_create("ufshpb_req_cache", >> >> >> >> > sizeof(struct ufshpb_req), 0, 0, NULL); >> >> >> >> > @@ -2114,8 +2153,10 @@ static void >> ufshpb_discard_rsp_lists(struct >> >> >> >> > ufshpb_lu *hpb) >> >> >> >> > >> >> >> >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) >> >> >> >> > { >> >> >> >> > - if (hpb->is_hcm) >> >> >> >> > + if (hpb->is_hcm) { >> >> >> >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); >> >> >> >> > cancel_work_sync(&hpb->ufshpb_normalization_work); >> >> >> >> > + } >> >> >> >> > cancel_work_sync(&hpb->map_work); >> >> >> >> > } >> >> >> >> > >> >> >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> >> >> >> > index 84598a317897..37c1b0ea0c0a 100644 >> >> >> >> > --- a/drivers/scsi/ufs/ufshpb.h >> >> >> >> > +++ b/drivers/scsi/ufs/ufshpb.h >> >> >> >> > @@ -121,6 +121,7 @@ struct ufshpb_region { >> >> >> >> > struct list_head list_lru_rgn; >> >> >> >> > unsigned long rgn_flags; >> >> >> >> > #define RGN_FLAG_DIRTY 0 >> >> >> >> > +#define RGN_FLAG_UPDATE 1 >> >> >> >> > >> >> >> >> > /* region reads - for host mode */ >> >> >> >> > spinlock_t rgn_lock; >> >> >> >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { >> >> >> >> > /* for selecting victim */ >> >> >> >> > struct victim_select_info lru_info; >> >> >> >> > struct work_struct ufshpb_normalization_work; >> >> >> >> > + struct work_struct ufshpb_lun_reset_work; >> >> >> >> > >> >> >> >> > /* pinned region information */ >> >> >> >> > u32 lu_pinned_start;
> On 2021-03-17 23:46, Avri Altman wrote: > >> >> >> >> > >> >> >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu() > >> >> >> >> does > >> >> >> >> not > >> >> >> >> seem a problem to me, does this really deserve a separate work? > >> >> >> > I don't know, I never even consider of doing this. > >> >> >> > The active region list may contain up to few thousands of regions - > >> >> >> > It is not rare to see configurations that covers the entire device. > >> >> >> > > >> >> >> > >> >> >> Yes, true, it can be a huge list. But what does the ops > >> >> >> "HPB_RSP_DEV_RESET" > >> >> >> really mean? The specs says "Device reset HPB Regions information", > >> >> >> but > >> >> >> I > >> >> >> don't know what is really happening. Could you please elaborate? > >> >> > It means that the device informs the host that the L2P cache is no > >> >> > longer valid. > >> >> > The spec doesn't say what to do in that case. > >> >> > >> >> Then it means that all the clean (without DIRTY flag set) HPB entries > >> >> (ppns) > >> >> in active rgns in host memory side may not be valid to the device > >> >> anymore. > >> >> Please correct me if I am wrong. > >> >> > >> >> > We thought that in host mode, it make sense to update all the active > >> >> > regions. > >> >> > >> >> But current logic does not set the state of the sub-regions (in active > >> >> regions) to > >> >> INVALID, it only marks all active regions as UPDATE. > >> >> > >> >> Although one of subsequent read cmds shall put the sub-region back to > >> >> activate_list, > >> >> ufshpb_test_ppn_dirty() can still return false, thus these read cmds > >> >> still think the > >> >> ppns are valid and they shall move forward to send HPB Write Buffer > >> >> (buffer id = 0x2, > >> >> in case of HPB2.0) and HPB Read cmds. > >> >> > >> >> HPB Read cmds with invalid ppns will be treated as normal Read cmds by > >> >> device as the > >> >> specs says, but what would happen to HPB Write Buffer cmds (buffer id > >> >> = > >> >> 0x2, in case > >> >> of HPB2.0) with invalid ppns? Can this be a real problem? > >> > No need to control the ppn dirty / invalid state for this case. > >> > The device send device reset so it is aware that all the L2P cache is > >> > invalid. > >> > Any HPB_READ is treated like normal READ10. > >> > > >> > Only once HPB-READ-BUFFER is completed, > >> > the device will relate back to the physical address. > >> > >> What about HPB-WRITE-BUFFER (buffer id = 0x2) cmds? > > Same. > > Oper 0x2 is a relative simple case. > > The device is expected to manage some versioning framework not to be > > "fooled" by erroneous ppn. > > There are some more challenging races that the device should meet. > > > > But I don't find the handling w.r.t this scenario on HPB2.0 specs - > how would the device re-act/respond to HPB-WRITE-BUFFER cmds with > invalid HPB entries? Could you please point me to relevant > section/paragraph? The spec does not handle that. HPB-WRITE-BUFFER 0x2 is not a stand-alone command, it always tagged to a HPB-READ command. It is up to the device to handle invalid ppn and always return the correct data. The expected performance in that case is like a regular READ10. Thanks, Avri > > Thanks, > Can Guo. > > > Thanks, > > Avri > >> > >> Thanks, > >> Can Guo. > >> > >> > > >> >> > >> >> > > >> >> > I think I will go with your suggestion. > >> >> > Effectively, in host mode, since it is deactivating "cold" regions, > >> >> > the lru list is kept relatively small, and contains only "hot" regions. > >> >> > >> >> hmm... I don't really have a idea on this, please go with whatever you > >> >> and Daejun think is fine here. > >> > I will take your advice and remove the worker. > >> > > >> > > >> > Thanks, > >> > Avri > >> > > >> >> > >> >> Thanks, > >> >> Can Guo. > >> >> > >> >> > > >> >> > Thanks, > >> >> > Avri > >> >> > > >> >> >> > >> >> >> Thanks, > >> >> >> Can Guo. > >> >> >> > >> >> >> > But yes, I can do that. > >> >> >> > Better to get ack from Daejun first. > >> >> >> > > >> >> >> > Thanks, > >> >> >> > Avri > >> >> >> > > >> >> >> >> > >> >> >> >> Thanks, > >> >> >> >> Can Guo. > >> >> >> >> > >> >> >> >> > +{ > >> >> >> >> > + struct ufshpb_lu *hpb; > >> >> >> >> > + struct victim_select_info *lru_info; > >> >> >> >> > + struct ufshpb_region *rgn; > >> >> >> >> > + unsigned long flags; > >> >> >> >> > + > >> >> >> >> > + hpb = container_of(work, struct ufshpb_lu, > >> >> ufshpb_lun_reset_work); > >> >> >> >> > + > >> >> >> >> > + lru_info = &hpb->lru_info; > >> >> >> >> > + > >> >> >> >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > >> >> >> >> > + > >> >> >> >> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) > >> >> >> >> > + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); > >> >> >> >> > + > >> >> >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > >> >> >> >> > +} > >> >> >> >> > + > >> >> >> >> > static void ufshpb_normalization_work_handler(struct > work_struct > >> >> >> >> > *work) > >> >> >> >> > { > >> >> >> >> > struct ufshpb_lu *hpb; > >> >> >> >> > @@ -1798,6 +1832,8 @@ static int > ufshpb_alloc_region_tbl(struct > >> >> >> >> > ufs_hba *hba, struct ufshpb_lu *hpb) > >> >> >> >> > } else { > >> >> >> >> > rgn->rgn_state = HPB_RGN_INACTIVE; > >> >> >> >> > } > >> >> >> >> > + > >> >> >> >> > + rgn->rgn_flags = 0; > >> >> >> >> > } > >> >> >> >> > > >> >> >> >> > return 0; > >> >> >> >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct > >> >> ufs_hba > >> >> >> >> > *hba, struct ufshpb_lu *hpb) > >> >> >> >> > INIT_LIST_HEAD(&hpb->list_hpb_lu); > >> >> >> >> > > >> >> >> >> > INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); > >> >> >> >> > - if (hpb->is_hcm) > >> >> >> >> > + if (hpb->is_hcm) { > >> >> >> >> > INIT_WORK(&hpb->ufshpb_normalization_work, > >> >> >> >> > ufshpb_normalization_work_handler); > >> >> >> >> > + INIT_WORK(&hpb->ufshpb_lun_reset_work, > >> >> >> >> > + ufshpb_reset_work_handler); > >> >> >> >> > + } > >> >> >> >> > > >> >> >> >> > hpb->map_req_cache = > >> kmem_cache_create("ufshpb_req_cache", > >> >> >> >> > sizeof(struct ufshpb_req), 0, 0, NULL); > >> >> >> >> > @@ -2114,8 +2153,10 @@ static void > >> ufshpb_discard_rsp_lists(struct > >> >> >> >> > ufshpb_lu *hpb) > >> >> >> >> > > >> >> >> >> > static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) > >> >> >> >> > { > >> >> >> >> > - if (hpb->is_hcm) > >> >> >> >> > + if (hpb->is_hcm) { > >> >> >> >> > + cancel_work_sync(&hpb->ufshpb_lun_reset_work); > >> >> >> >> > cancel_work_sync(&hpb->ufshpb_normalization_work); > >> >> >> >> > + } > >> >> >> >> > cancel_work_sync(&hpb->map_work); > >> >> >> >> > } > >> >> >> >> > > >> >> >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > >> >> >> >> > index 84598a317897..37c1b0ea0c0a 100644 > >> >> >> >> > --- a/drivers/scsi/ufs/ufshpb.h > >> >> >> >> > +++ b/drivers/scsi/ufs/ufshpb.h > >> >> >> >> > @@ -121,6 +121,7 @@ struct ufshpb_region { > >> >> >> >> > struct list_head list_lru_rgn; > >> >> >> >> > unsigned long rgn_flags; > >> >> >> >> > #define RGN_FLAG_DIRTY 0 > >> >> >> >> > +#define RGN_FLAG_UPDATE 1 > >> >> >> >> > > >> >> >> >> > /* region reads - for host mode */ > >> >> >> >> > spinlock_t rgn_lock; > >> >> >> >> > @@ -217,6 +218,7 @@ struct ufshpb_lu { > >> >> >> >> > /* for selecting victim */ > >> >> >> >> > struct victim_select_info lru_info; > >> >> >> >> > struct work_struct ufshpb_normalization_work; > >> >> >> >> > + struct work_struct ufshpb_lun_reset_work; > >> >> >> >> > > >> >> >> >> > /* pinned region information */ > >> >> >> >> > u32 lu_pinned_start;
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c index 0744feb4d484..0034fa03fdc6 100644 --- a/drivers/scsi/ufs/ufshpb.c +++ b/drivers/scsi/ufs/ufshpb.c @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) if (rgn->reads == ACTIVATION_THRESHOLD) activate = true; spin_unlock_irqrestore(&rgn->rgn_lock, flags); - if (activate) { + if (activate || + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) { spin_lock_irqsave(&hpb->rsp_list_lock, flags); ufshpb_update_active_info(hpb, rgn_idx, srgn_idx); hpb->stats.rb_active_cnt++; @@ -1480,6 +1481,20 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) case HPB_RSP_DEV_RESET: dev_warn(&hpb->sdev_ufs_lu->sdev_dev, "UFS device lost HPB information during PM.\n"); + + if (hpb->is_hcm) { + struct scsi_device *sdev; + + __shost_for_each_device(sdev, hba->host) { + struct ufshpb_lu *h = sdev->hostdata; + + if (!h) + continue; + + schedule_work(&hpb->ufshpb_lun_reset_work); + } + } + break; default: dev_notice(&hpb->sdev_ufs_lu->sdev_dev, @@ -1594,6 +1609,25 @@ static void ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb) spin_unlock_irqrestore(&hpb->rsp_list_lock, flags); } +static void ufshpb_reset_work_handler(struct work_struct *work) +{ + struct ufshpb_lu *hpb; + struct victim_select_info *lru_info; + struct ufshpb_region *rgn; + unsigned long flags; + + hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work); + + lru_info = &hpb->lru_info; + + spin_lock_irqsave(&hpb->rgn_state_lock, flags); + + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) + set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags); + + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); +} + static void ufshpb_normalization_work_handler(struct work_struct *work) { struct ufshpb_lu *hpb; @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb) } else { rgn->rgn_state = HPB_RGN_INACTIVE; } + + rgn->rgn_flags = 0; } return 0; @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb) INIT_LIST_HEAD(&hpb->list_hpb_lu); INIT_WORK(&hpb->map_work, ufshpb_map_work_handler); - if (hpb->is_hcm) + if (hpb->is_hcm) { INIT_WORK(&hpb->ufshpb_normalization_work, ufshpb_normalization_work_handler); + INIT_WORK(&hpb->ufshpb_lun_reset_work, + ufshpb_reset_work_handler); + } hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache", sizeof(struct ufshpb_req), 0, 0, NULL); @@ -2114,8 +2153,10 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb) static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb) { - if (hpb->is_hcm) + if (hpb->is_hcm) { + cancel_work_sync(&hpb->ufshpb_lun_reset_work); cancel_work_sync(&hpb->ufshpb_normalization_work); + } cancel_work_sync(&hpb->map_work); } diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h index 84598a317897..37c1b0ea0c0a 100644 --- a/drivers/scsi/ufs/ufshpb.h +++ b/drivers/scsi/ufs/ufshpb.h @@ -121,6 +121,7 @@ struct ufshpb_region { struct list_head list_lru_rgn; unsigned long rgn_flags; #define RGN_FLAG_DIRTY 0 +#define RGN_FLAG_UPDATE 1 /* region reads - for host mode */ spinlock_t rgn_lock; @@ -217,6 +218,7 @@ struct ufshpb_lu { /* for selecting victim */ struct victim_select_info lru_info; struct work_struct ufshpb_normalization_work; + struct work_struct ufshpb_lun_reset_work; /* pinned region information */ u32 lu_pinned_start;
The spec does not define what is the host's recommended response when the device send hpb dev reset response (oper 0x2). We will update all active hpb regions: mark them and do that on the next read. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshpb.c | 47 ++++++++++++++++++++++++++++++++++++--- drivers/scsi/ufs/ufshpb.h | 2 ++ 2 files changed, 46 insertions(+), 3 deletions(-)