RE: [PATCH v5 06/10] scsi: ufshpb: Add hpb dev reset response

[Avri Altman <Avri.Altman@wdc.com>]

> 
> 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;