> > The data structure for map data request and L2P map uses mempool API,
> > minimizing allocation overhead while avoiding static allocation.
> Maybe one or two more sentences to explain the L2P framework:
> Each hpb lun maintains 2 "to-do" lists:
> - hpb->lh_inact_rgn - regions to be inactivated, and
> - hpb->lh_act_srgn - subregions to be activated
> Those lists are being checked on every resume and completion interrupt.
OK, I will add more description of L2P framework.
> >
> > Signed-off-by: Daejun Park <daejun7.park@xxxxxxxxxxx>
> > ---
> > + for (i = 0; i < hpb->pages_per_srgn; i++) {
> > + mctx->m_page[i] = mempool_alloc(ufshpb_drv.ufshpb_page_pool,
> > + GFP_KERNEL);
> > + memset(page_address(mctx->m_page[i]), 0, PAGE_SIZE);
> Better move this memset after if (!mctx->m_page[i]).
> And maybe use clear_page instead?
OK, I will change the code.
> > + if (!mctx->m_page[i]) {
> > + for (j = 0; j < i; j++)
> > + mempool_free(mctx->m_page[j],
> > + ufshpb_drv.ufshpb_page_pool);
> > + goto release_ppn_dirty;
> > + }
> > +static inline int ufshpb_add_region(struct ufshpb_lu *hpb,
> > + struct ufshpb_region *rgn)
> > +{
> Maybe better describe what this function does - ufshpb_get_rgn_map_ctx ?
Yes, I think "ufshpb_get_rgn_map_ctx" is better name.
> > + if (!list_empty(&rgn->list_lru_rgn)) {
> > + if (ufshpb_check_issue_state_srgns(hpb, rgn)) {
> So if one of its subregions has inflight map request - you add it to the "starved" list?
> Why call it starved?
"starved list" was wrong name. I will change it to "postponed_evict_list".
> > + * Since the region state change occurs only in the hpb task-work,
> > + * the state of the region cannot HPB_RGN_INACTIVE at this point.
> > + * The region state must be changed in the hpb task-work
> I think that you called this worker map_work?
Yes, "the hpb task-work" will be changed to the map_work.
> > + spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> > + ret = ufshpb_add_region(hpb, rgn);
> If this is not an active region,
> Although the device indicated to activate a specific subregion,
> You are activating all the subregions of that region.
> You should elaborate on that in your commit log,
> and explain why this is the correct activation course.
Yes, I'm going to change the code to activate only the subregions that are "activate state".
> get_unaligned instead of be16_to_cpu ?
Yes, I will change.
> > +
> > + if (!data_seg_len) {
> data_seg_len should be DEV_DATA_SEG_LEN, and you should also check HPB_UPDATE_ALERT,
> which you might want to do here and not in ufshpb_may_field_valid
Yes, I will change.
> > + switch (rsp_field->hpb_type) {
> > + case HPB_RSP_REQ_REGION_UPDATE:
> > + WARN_ON(data_seg_len != DEV_DATA_SEG_LEN);
> > + ufshpb_rsp_req_region_update(hpb, rsp_field);
> > + break;
> What about hpb dev reset - oper 0x2?
Yes, I will change.
> > +static void ufshpb_add_active_list(struct ufshpb_lu *hpb,
> > + struct ufshpb_region *rgn,
> > + struct ufshpb_subregion *srgn)
> > +{
> > + if (!list_empty(&rgn->list_inact_rgn))
> > + return;
> > +
> > + if (!list_empty(&srgn->list_act_srgn)) {
> > + list_move(&srgn->list_act_srgn, &hpb->lh_act_srgn);
> Why is this needed?
> Why updating this subregion position?
The "ufshpb_add_active_list()" is called from "ufshpb_run_active_subregion_list()" to retry activating subregion that failed to activate.
Therefore, it requeues the subregion to activate region list head.
> > @@ -195,8 +1047,15 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba
> > *hba, struct ufshpb_lu *hpb)
> > release_srgn_table:
> > for (i = 0; i < rgn_idx; i++) {
> > rgn = rgn_table + i;
> > - if (rgn->srgn_tbl)
> > + if (rgn->srgn_tbl) {
> > + for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt;
> > + srgn_idx++) {
> > + srgn = rgn->srgn_tbl + srgn_idx;
> > + if (srgn->mctx)
> How is it even possible that on init there is an active subregion?
> ufshpb_init_pinned_active_region does its own cleanup.
I will fix the duplicated cleanup codes.
> > + hpb->m_page_cache = kmem_cache_create("ufshpb_m_page_cache",
> > + sizeof(struct page *) * hpb->pages_per_srgn,
> > + 0, 0, NULL);
> What is the advantage in using an array of page pointers,
> Instead of a single pointer to pages_per_srgn?
To minimize memory fragmentation problem, I used pointer + single page rather than single array of pages.
> > @@ -398,6 +1326,9 @@ static void ufshpb_resume(struct ufs_hba *hba)
> >
> > dev_info(&hpb->hpb_lu_dev, "ufshpb resume");
> > ufshpb_set_state(hpb, HPB_PRESENT);
> > + if (!ufshpb_is_empty_rsp_lists(hpb))
> > + queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
> Ahha - so you are using the ufs driver pm flows to poll your work queue.
> Why device recommendations isn't enough?
I don't understand this comment. The code resumes map_work that was stopped by PM during the map request.
Please explain your concerns.
Thanks,
Avri
