RE: [PATCH v6 02/10] scsi: ufshpb: Add host control mode support to rsp_upiu

[Avri Altman]

> >> @@ -1245,6 +1257,18 @@ static void
> ufshpb_rsp_req_region_update(struct
> >> ufshpb_lu *hpb,
> >>              srgn_i =
> >>                      be16_to_cpu(rsp_field->hpb_active_field[i].active_srgn);
> >>
> >> +            rgn = hpb->rgn_tbl + rgn_i;
> >> +            if (hpb->is_hcm &&
> >> +                (rgn->rgn_state != HPB_RGN_ACTIVE || is_rgn_dirty(rgn))) {
> >> +                    /*
> >> +                     * in host control mode, subregion activation
> >> +                     * recommendations are only allowed to active regions.
> >> +                     * Also, ignore recommendations for dirty regions - the
> >> +                     * host will make decisions concerning those by himself
> >> +                     */
> >> +                    continue;
> >> +            }
> >> +
> >
> > Hi Avri, host control mode also need the recommendations from device,
> > because the bkops would make the ppn invalid, is that right?
> 
> Right, but ONLY recommandations to ACTIVE regions are of host's interest
> in host control mode. For those inactive regions, host makes its own
> decisions.
Correct.
Generally speaking, in host control mode, the host may ignore the device recommendation altogether.
In host mode the device is not allowed to send recommendation to Inactive regions.
Furthermore, as the comment above indicates, we *chose* to ignore recommendation for DIRTY regions.
We do so, because the host is aware of dirty regions and can make the decision per its own internal logic.
ONLY if the region is ACTIVE and CLEAN - the host is unaware that bkops took place,
So we *chose* to act per the device recommendation.  

Thanks,
Avri
> 
> Can Guo.
> 
> >
> >>              dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
> >>                      "activate(%d) region %d - %d\n", i, rgn_i, srgn_i);
> >>
> >> @@ -1252,7 +1276,6 @@ static void ufshpb_rsp_req_region_update(struct
> >> ufshpb_lu *hpb,
> >>              ufshpb_update_active_info(hpb, rgn_i, srgn_i);
> >>              spin_unlock(&hpb->rsp_list_lock);
> >>
> >> -            rgn = hpb->rgn_tbl + rgn_i;
> >>              srgn = rgn->srgn_tbl + srgn_i;
> >>
> >>              /* blocking HPB_READ */
> >> @@ -1263,6 +1286,14 @@ static void
> ufshpb_rsp_req_region_update(struct
> >> ufshpb_lu *hpb,
> >>              hpb->stats.rb_active_cnt++;
> >>      }
> >>
> >> +    if (hpb->is_hcm) {
> >> +            /*
> >> +             * in host control mode the device is not allowed to inactivate
> >> +             * regions
> >> +             */
> >> +            goto out;
> >> +    }
> >> +
> >>      for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
> >>              rgn_i = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
> >>              dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
> >> @@ -1287,6 +1318,7 @@ static void ufshpb_rsp_req_region_update(struct
> >> ufshpb_lu *hpb,
> >>              hpb->stats.rb_inactive_cnt++;
> >>      }
> >>
> >> +out:
> >>      dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "Noti: #ACT %u #INACT %u\n",
> >>              rsp_field->active_rgn_cnt, rsp_field->inactive_rgn_cnt);
> >>
> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> >> index 7df30340386a..032672114881 100644
> >> --- a/drivers/scsi/ufs/ufshpb.h
> >> +++ b/drivers/scsi/ufs/ufshpb.h
> >> @@ -121,6 +121,8 @@ struct ufshpb_region {
> >>
> >>      /* below information is used by lru */
> >>      struct list_head list_lru_rgn;
> >> +    unsigned long rgn_flags;
> >> +#define RGN_FLAG_DIRTY 0
> >>  };
> >>
> >>  #define for_each_sub_region(rgn, i, srgn)                           \
> >> --
> >> 2.25.1
> >>