| Message ID | 20210331073952.102162-7-avri.altman@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Host control mode to HPB | expand |
On 2021-03-31 15:39, Avri Altman wrote: > In host mode, the host is expected to send HPB-WRITE-BUFFER with > buffer-id = 0x1 when it inactivates a region. > > Use the map-requests pool as there is no point in assigning a > designated cache for umap-requests. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/ufs/ufshpb.c | 35 +++++++++++++++++++++++++++++++---- > drivers/scsi/ufs/ufshpb.h | 1 + > 2 files changed, 32 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index aefb6dc160ee..fcc954f51bcf 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -914,6 +914,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu > *hpb, > > blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn); > > + hpb->stats.umap_req_cnt++; > return 0; > } > > @@ -1110,18 +1111,37 @@ static int ufshpb_issue_umap_req(struct > ufshpb_lu *hpb, > return -EAGAIN; > } > > +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, > + struct ufshpb_region *rgn) > +{ > + return ufshpb_issue_umap_req(hpb, rgn); > +} > + > static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) > { > return ufshpb_issue_umap_req(hpb, NULL); > } > > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, > - struct ufshpb_region *rgn) > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, > + struct ufshpb_region *rgn) > { > struct victim_select_info *lru_info; > struct ufshpb_subregion *srgn; > int srgn_idx; > > + lockdep_assert_held(&hpb->rgn_state_lock); > + > + if (hpb->is_hcm) { > + unsigned long flags; > + int ret; > + > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); Never seen a usage like this... Here flags is used without being intialized. The flag is needed when spin_unlock_irqrestore -> local_irq_restore(flags) to restore the DAIF register (in terms of ARM). Thanks, Can Guo. > + ret = ufshpb_issue_umap_single_req(hpb, rgn); > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > + if (ret) > + return ret; > + } > + > lru_info = &hpb->lru_info; > > dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", > rgn->rgn_idx); > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct > ufshpb_lu *hpb, > > for_each_sub_region(rgn, srgn_idx, srgn) > ufshpb_purge_active_subregion(hpb, srgn); > + > + return 0; > } > > static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct > ufshpb_region *rgn) > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu > *hpb, struct ufshpb_region *rgn) > goto out; > } > > - __ufshpb_evict_region(hpb, rgn); > + ret = __ufshpb_evict_region(hpb, rgn); > } > out: > spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu > *hpb, struct ufshpb_region *rgn) > "LRU full (%d), choose victim %d\n", > atomic_read(&lru_info->active_cnt), > victim_rgn->rgn_idx); > - __ufshpb_evict_region(hpb, victim_rgn); > + ret = __ufshpb_evict_region(hpb, victim_rgn); > + if (ret) > + goto out; > } > > /* > @@ -1856,6 +1880,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); > ufshpb_sysfs_attr_show_func(rb_active_cnt); > ufshpb_sysfs_attr_show_func(rb_inactive_cnt); > ufshpb_sysfs_attr_show_func(map_req_cnt); > +ufshpb_sysfs_attr_show_func(umap_req_cnt); > > static struct attribute *hpb_dev_stat_attrs[] = { > &dev_attr_hit_cnt.attr, > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { > &dev_attr_rb_active_cnt.attr, > &dev_attr_rb_inactive_cnt.attr, > &dev_attr_map_req_cnt.attr, > + &dev_attr_umap_req_cnt.attr, > NULL, > }; > > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu > *hpb) > hpb->stats.rb_active_cnt = 0; > hpb->stats.rb_inactive_cnt = 0; > hpb->stats.map_req_cnt = 0; > + hpb->stats.umap_req_cnt = 0; > } > > static void ufshpb_param_init(struct ufshpb_lu *hpb) > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > index 87495e59fcf1..1ea58c17a4de 100644 > --- a/drivers/scsi/ufs/ufshpb.h > +++ b/drivers/scsi/ufs/ufshpb.h > @@ -191,6 +191,7 @@ struct ufshpb_stats { > u64 rb_inactive_cnt; > u64 map_req_cnt; > u64 pre_req_cnt; > + u64 umap_req_cnt; > }; > > struct ufshpb_lu {
> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, > > - struct ufshpb_region *rgn) > > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, > > + struct ufshpb_region *rgn) > > { > > struct victim_select_info *lru_info; > > struct ufshpb_subregion *srgn; > > int srgn_idx; > > > > + lockdep_assert_held(&hpb->rgn_state_lock); > > + > > + if (hpb->is_hcm) { > > + unsigned long flags; > > + int ret; > > + > > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > > Never seen a usage like this... Here flags is used without being > intialized. > The flag is needed when spin_unlock_irqrestore -> > local_irq_restore(flags) to > restore the DAIF register (in terms of ARM). OK. Thanks, Avri > > Thanks, > > Can Guo. > > > + ret = ufshpb_issue_umap_single_req(hpb, rgn); > > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > > + if (ret) > > + return ret; > > + } > > + > > lru_info = &hpb->lru_info; > > > > dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", > > rgn->rgn_idx); > > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct > > ufshpb_lu *hpb, > > > > for_each_sub_region(rgn, srgn_idx, srgn) > > ufshpb_purge_active_subregion(hpb, srgn); > > + > > + return 0; > > } > > > > static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct > > ufshpb_region *rgn) > > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu > > *hpb, struct ufshpb_region *rgn) > > goto out; > > } > > > > - __ufshpb_evict_region(hpb, rgn); > > + ret = __ufshpb_evict_region(hpb, rgn); > > } > > out: > > spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu > > *hpb, struct ufshpb_region *rgn) > > "LRU full (%d), choose victim %d\n", > > atomic_read(&lru_info->active_cnt), > > victim_rgn->rgn_idx); > > - __ufshpb_evict_region(hpb, victim_rgn); > > + ret = __ufshpb_evict_region(hpb, victim_rgn); > > + if (ret) > > + goto out; > > } > > > > /* > > @@ -1856,6 +1880,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); > > ufshpb_sysfs_attr_show_func(rb_active_cnt); > > ufshpb_sysfs_attr_show_func(rb_inactive_cnt); > > ufshpb_sysfs_attr_show_func(map_req_cnt); > > +ufshpb_sysfs_attr_show_func(umap_req_cnt); > > > > static struct attribute *hpb_dev_stat_attrs[] = { > > &dev_attr_hit_cnt.attr, > > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { > > &dev_attr_rb_active_cnt.attr, > > &dev_attr_rb_inactive_cnt.attr, > > &dev_attr_map_req_cnt.attr, > > + &dev_attr_umap_req_cnt.attr, > > NULL, > > }; > > > > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu > > *hpb) > > hpb->stats.rb_active_cnt = 0; > > hpb->stats.rb_inactive_cnt = 0; > > hpb->stats.map_req_cnt = 0; > > + hpb->stats.umap_req_cnt = 0; > > } > > > > static void ufshpb_param_init(struct ufshpb_lu *hpb) > > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > > index 87495e59fcf1..1ea58c17a4de 100644 > > --- a/drivers/scsi/ufs/ufshpb.h > > +++ b/drivers/scsi/ufs/ufshpb.h > > @@ -191,6 +191,7 @@ struct ufshpb_stats { > > u64 rb_inactive_cnt; > > u64 map_req_cnt; > > u64 pre_req_cnt; > > + u64 umap_req_cnt; > > }; > > > > struct ufshpb_lu {
On 2021-04-06 13:20, Avri Altman wrote: >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, >> > - struct ufshpb_region *rgn) >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, >> > + struct ufshpb_region *rgn) >> > { >> > struct victim_select_info *lru_info; >> > struct ufshpb_subregion *srgn; >> > int srgn_idx; >> > >> > + lockdep_assert_held(&hpb->rgn_state_lock); >> > + >> > + if (hpb->is_hcm) { >> > + unsigned long flags; >> > + int ret; >> > + >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> >> Never seen a usage like this... Here flags is used without being >> intialized. >> The flag is needed when spin_unlock_irqrestore -> >> local_irq_restore(flags) to >> restore the DAIF register (in terms of ARM). > OK. Hi Avri, Checked on my setup, this lead to compilation error. Will you fix it in next version? warning: variable 'flags' is uninitialized when used here [-Wuninitialized] Thanks, Can Guo. > > Thanks, > Avri > >> >> Thanks, >> >> Can Guo. >> >> > + ret = ufshpb_issue_umap_single_req(hpb, rgn); >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); >> > + if (ret) >> > + return ret; >> > + } >> > + >> > lru_info = &hpb->lru_info; >> > >> > dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >> > rgn->rgn_idx); >> > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct >> > ufshpb_lu *hpb, >> > >> > for_each_sub_region(rgn, srgn_idx, srgn) >> > ufshpb_purge_active_subregion(hpb, srgn); >> > + >> > + return 0; >> > } >> > >> > static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct >> > ufshpb_region *rgn) >> > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu >> > *hpb, struct ufshpb_region *rgn) >> > goto out; >> > } >> > >> > - __ufshpb_evict_region(hpb, rgn); >> > + ret = __ufshpb_evict_region(hpb, rgn); >> > } >> > out: >> > spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu >> > *hpb, struct ufshpb_region *rgn) >> > "LRU full (%d), choose victim %d\n", >> > atomic_read(&lru_info->active_cnt), >> > victim_rgn->rgn_idx); >> > - __ufshpb_evict_region(hpb, victim_rgn); >> > + ret = __ufshpb_evict_region(hpb, victim_rgn); >> > + if (ret) >> > + goto out; >> > } >> > >> > /* >> > @@ -1856,6 +1880,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >> > ufshpb_sysfs_attr_show_func(rb_active_cnt); >> > ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >> > ufshpb_sysfs_attr_show_func(map_req_cnt); >> > +ufshpb_sysfs_attr_show_func(umap_req_cnt); >> > >> > static struct attribute *hpb_dev_stat_attrs[] = { >> > &dev_attr_hit_cnt.attr, >> > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { >> > &dev_attr_rb_active_cnt.attr, >> > &dev_attr_rb_inactive_cnt.attr, >> > &dev_attr_map_req_cnt.attr, >> > + &dev_attr_umap_req_cnt.attr, >> > NULL, >> > }; >> > >> > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >> > *hpb) >> > hpb->stats.rb_active_cnt = 0; >> > hpb->stats.rb_inactive_cnt = 0; >> > hpb->stats.map_req_cnt = 0; >> > + hpb->stats.umap_req_cnt = 0; >> > } >> > >> > static void ufshpb_param_init(struct ufshpb_lu *hpb) >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> > index 87495e59fcf1..1ea58c17a4de 100644 >> > --- a/drivers/scsi/ufs/ufshpb.h >> > +++ b/drivers/scsi/ufs/ufshpb.h >> > @@ -191,6 +191,7 @@ struct ufshpb_stats { >> > u64 rb_inactive_cnt; >> > u64 map_req_cnt; >> > u64 pre_req_cnt; >> > + u64 umap_req_cnt; >> > }; >> > >> > struct ufshpb_lu {
> > On 2021-04-06 13:20, Avri Altman wrote: > >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, > >> > - struct ufshpb_region *rgn) > >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, > >> > + struct ufshpb_region *rgn) > >> > { > >> > struct victim_select_info *lru_info; > >> > struct ufshpb_subregion *srgn; > >> > int srgn_idx; > >> > > >> > + lockdep_assert_held(&hpb->rgn_state_lock); > >> > + > >> > + if (hpb->is_hcm) { > >> > + unsigned long flags; > >> > + int ret; > >> > + > >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > >> > >> Never seen a usage like this... Here flags is used without being > >> intialized. > >> The flag is needed when spin_unlock_irqrestore -> > >> local_irq_restore(flags) to > >> restore the DAIF register (in terms of ARM). > > OK. > > Hi Avri, > > Checked on my setup, this lead to compilation error. Will you fix it in > next version? > > warning: variable 'flags' is uninitialized when used here > [-Wuninitialized] Yeah - I will pass it to __ufshpb_evict_region and drop the lockdep_assert call. I don't want to block your testing - are there any other things you want me to change? Thanks, Avri > > Thanks, > Can Guo. > > > > > Thanks, > > Avri > > > >> > >> Thanks, > >> > >> Can Guo. > >> > >> > + ret = ufshpb_issue_umap_single_req(hpb, rgn); > >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); > >> > + if (ret) > >> > + return ret; > >> > + } > >> > + > >> > lru_info = &hpb->lru_info; > >> > > >> > dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", > >> > rgn->rgn_idx); > >> > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct > >> > ufshpb_lu *hpb, > >> > > >> > for_each_sub_region(rgn, srgn_idx, srgn) > >> > ufshpb_purge_active_subregion(hpb, srgn); > >> > + > >> > + return 0; > >> > } > >> > > >> > static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct > >> > ufshpb_region *rgn) > >> > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu > >> > *hpb, struct ufshpb_region *rgn) > >> > goto out; > >> > } > >> > > >> > - __ufshpb_evict_region(hpb, rgn); > >> > + ret = __ufshpb_evict_region(hpb, rgn); > >> > } > >> > out: > >> > spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > >> > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu > >> > *hpb, struct ufshpb_region *rgn) > >> > "LRU full (%d), choose victim %d\n", > >> > atomic_read(&lru_info->active_cnt), > >> > victim_rgn->rgn_idx); > >> > - __ufshpb_evict_region(hpb, victim_rgn); > >> > + ret = __ufshpb_evict_region(hpb, victim_rgn); > >> > + if (ret) > >> > + goto out; > >> > } > >> > > >> > /* > >> > @@ -1856,6 +1880,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); > >> > ufshpb_sysfs_attr_show_func(rb_active_cnt); > >> > ufshpb_sysfs_attr_show_func(rb_inactive_cnt); > >> > ufshpb_sysfs_attr_show_func(map_req_cnt); > >> > +ufshpb_sysfs_attr_show_func(umap_req_cnt); > >> > > >> > static struct attribute *hpb_dev_stat_attrs[] = { > >> > &dev_attr_hit_cnt.attr, > >> > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { > >> > &dev_attr_rb_active_cnt.attr, > >> > &dev_attr_rb_inactive_cnt.attr, > >> > &dev_attr_map_req_cnt.attr, > >> > + &dev_attr_umap_req_cnt.attr, > >> > NULL, > >> > }; > >> > > >> > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu > >> > *hpb) > >> > hpb->stats.rb_active_cnt = 0; > >> > hpb->stats.rb_inactive_cnt = 0; > >> > hpb->stats.map_req_cnt = 0; > >> > + hpb->stats.umap_req_cnt = 0; > >> > } > >> > > >> > static void ufshpb_param_init(struct ufshpb_lu *hpb) > >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > >> > index 87495e59fcf1..1ea58c17a4de 100644 > >> > --- a/drivers/scsi/ufs/ufshpb.h > >> > +++ b/drivers/scsi/ufs/ufshpb.h > >> > @@ -191,6 +191,7 @@ struct ufshpb_stats { > >> > u64 rb_inactive_cnt; > >> > u64 map_req_cnt; > >> > u64 pre_req_cnt; > >> > + u64 umap_req_cnt; > >> > }; > >> > > >> > struct ufshpb_lu {
On 2021-04-06 14:16, Avri Altman wrote: >> >> On 2021-04-06 13:20, Avri Altman wrote: >> >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, >> >> > - struct ufshpb_region *rgn) >> >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, >> >> > + struct ufshpb_region *rgn) >> >> > { >> >> > struct victim_select_info *lru_info; >> >> > struct ufshpb_subregion *srgn; >> >> > int srgn_idx; >> >> > >> >> > + lockdep_assert_held(&hpb->rgn_state_lock); >> >> > + >> >> > + if (hpb->is_hcm) { >> >> > + unsigned long flags; >> >> > + int ret; >> >> > + >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> >> >> >> Never seen a usage like this... Here flags is used without being >> >> intialized. >> >> The flag is needed when spin_unlock_irqrestore -> >> >> local_irq_restore(flags) to >> >> restore the DAIF register (in terms of ARM). >> > OK. >> >> Hi Avri, >> >> Checked on my setup, this lead to compilation error. Will you fix it >> in >> next version? >> >> warning: variable 'flags' is uninitialized when used here >> [-Wuninitialized] > Yeah - I will pass it to __ufshpb_evict_region and drop the > lockdep_assert call. > Please paste the sample code/change here so that I can move forward quickly. > I don't want to block your testing - are there any other things you > want me to change? Currently, no. I will try to review and test this series these days and post comments at once. Thanks, Can Guo. > > Thanks, > Avri > >> >> Thanks, >> Can Guo. >> >> > >> > Thanks, >> > Avri >> > >> >> >> >> Thanks, >> >> >> >> Can Guo. >> >> >> >> > + ret = ufshpb_issue_umap_single_req(hpb, rgn); >> >> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags); >> >> > + if (ret) >> >> > + return ret; >> >> > + } >> >> > + >> >> > lru_info = &hpb->lru_info; >> >> > >> >> > dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >> >> > rgn->rgn_idx); >> >> > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct >> >> > ufshpb_lu *hpb, >> >> > >> >> > for_each_sub_region(rgn, srgn_idx, srgn) >> >> > ufshpb_purge_active_subregion(hpb, srgn); >> >> > + >> >> > + return 0; >> >> > } >> >> > >> >> > static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct >> >> > ufshpb_region *rgn) >> >> > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu >> >> > *hpb, struct ufshpb_region *rgn) >> >> > goto out; >> >> > } >> >> > >> >> > - __ufshpb_evict_region(hpb, rgn); >> >> > + ret = __ufshpb_evict_region(hpb, rgn); >> >> > } >> >> > out: >> >> > spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); >> >> > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu >> >> > *hpb, struct ufshpb_region *rgn) >> >> > "LRU full (%d), choose victim %d\n", >> >> > atomic_read(&lru_info->active_cnt), >> >> > victim_rgn->rgn_idx); >> >> > - __ufshpb_evict_region(hpb, victim_rgn); >> >> > + ret = __ufshpb_evict_region(hpb, victim_rgn); >> >> > + if (ret) >> >> > + goto out; >> >> > } >> >> > >> >> > /* >> >> > @@ -1856,6 +1880,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >> >> > ufshpb_sysfs_attr_show_func(rb_active_cnt); >> >> > ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >> >> > ufshpb_sysfs_attr_show_func(map_req_cnt); >> >> > +ufshpb_sysfs_attr_show_func(umap_req_cnt); >> >> > >> >> > static struct attribute *hpb_dev_stat_attrs[] = { >> >> > &dev_attr_hit_cnt.attr, >> >> > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { >> >> > &dev_attr_rb_active_cnt.attr, >> >> > &dev_attr_rb_inactive_cnt.attr, >> >> > &dev_attr_map_req_cnt.attr, >> >> > + &dev_attr_umap_req_cnt.attr, >> >> > NULL, >> >> > }; >> >> > >> >> > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >> >> > *hpb) >> >> > hpb->stats.rb_active_cnt = 0; >> >> > hpb->stats.rb_inactive_cnt = 0; >> >> > hpb->stats.map_req_cnt = 0; >> >> > + hpb->stats.umap_req_cnt = 0; >> >> > } >> >> > >> >> > static void ufshpb_param_init(struct ufshpb_lu *hpb) >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> >> > index 87495e59fcf1..1ea58c17a4de 100644 >> >> > --- a/drivers/scsi/ufs/ufshpb.h >> >> > +++ b/drivers/scsi/ufs/ufshpb.h >> >> > @@ -191,6 +191,7 @@ struct ufshpb_stats { >> >> > u64 rb_inactive_cnt; >> >> > u64 map_req_cnt; >> >> > u64 pre_req_cnt; >> >> > + u64 umap_req_cnt; >> >> > }; >> >> > >> >> > struct ufshpb_lu {
> >> > >> On 2021-04-06 13:20, Avri Altman wrote: > >> >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, > >> >> > - struct ufshpb_region *rgn) > >> >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, > >> >> > + struct ufshpb_region *rgn) > >> >> > { > >> >> > struct victim_select_info *lru_info; > >> >> > struct ufshpb_subregion *srgn; > >> >> > int srgn_idx; > >> >> > > >> >> > + lockdep_assert_held(&hpb->rgn_state_lock); > >> >> > + > >> >> > + if (hpb->is_hcm) { > >> >> > + unsigned long flags; > >> >> > + int ret; > >> >> > + > >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > >> >> > >> >> Never seen a usage like this... Here flags is used without being > >> >> intialized. > >> >> The flag is needed when spin_unlock_irqrestore -> > >> >> local_irq_restore(flags) to > >> >> restore the DAIF register (in terms of ARM). > >> > OK. > >> > >> Hi Avri, > >> > >> Checked on my setup, this lead to compilation error. Will you fix it > >> in > >> next version? > >> > >> warning: variable 'flags' is uninitialized when used here > >> [-Wuninitialized] > > Yeah - I will pass it to __ufshpb_evict_region and drop the > > lockdep_assert call. > > > > Please paste the sample code/change here so that I can move forward > quickly. Thanks a lot. Also attaching the patch if its more convenient. Thanks, Avri $ git show 5d33d36e8704 commit 5d33d36e87047d27a546ad3529cb7837186b47b2 Author: Avri Altman <avri.altman@wdc.com> Date: Tue Jun 30 15:14:31 2020 +0300 scsi: ufshpb: Region inactivation in host mode In host mode, the host is expected to send HPB-WRITE-BUFFER with buffer-id = 0x1 when it inactivates a region. Use the map-requests pool as there is no point in assigning a designated cache for umap-requests. Signed-off-by: Avri Altman <avri.altman@wdc.com> Reviewed-by: Daejun Park <daejun7.park@samsung.com> Change-Id: I1a6696b38d4abfb4d9fbe44e84016a6238825125 diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c index aefb6dc160ee..54a3ea9f5732 100644 --- a/drivers/scsi/ufs/ufshpb.c +++ b/drivers/scsi/ufs/ufshpb.c @@ -914,6 +914,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb, blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn); + hpb->stats.umap_req_cnt++; return 0; } @@ -1110,18 +1111,35 @@ static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb, return -EAGAIN; } +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, + struct ufshpb_region *rgn) +{ + return ufshpb_issue_umap_req(hpb, rgn); +} + static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) { return ufshpb_issue_umap_req(hpb, NULL); } -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, - struct ufshpb_region *rgn) +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, + struct ufshpb_region *rgn, + unsigned long flags) { struct victim_select_info *lru_info; struct ufshpb_subregion *srgn; int srgn_idx; + if (hpb->is_hcm) { + int ret; + + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); + ret = ufshpb_issue_umap_single_req(hpb, rgn); + spin_lock_irqsave(&hpb->rgn_state_lock, flags); + if (ret) + return ret; + } + lru_info = &hpb->lru_info; dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx); @@ -1130,6 +1148,8 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb, for_each_sub_region(rgn, srgn_idx, srgn) ufshpb_purge_active_subregion(hpb, srgn); + + return 0; } static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn) @@ -1151,7 +1171,7 @@ static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn) goto out; } - __ufshpb_evict_region(hpb, rgn); + ret = __ufshpb_evict_region(hpb, rgn, flags); } out: spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); @@ -1285,7 +1305,9 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn) "LRU full (%d), choose victim %d\n", atomic_read(&lru_info->active_cnt), victim_rgn->rgn_idx); - __ufshpb_evict_region(hpb, victim_rgn); + ret = __ufshpb_evict_region(hpb, victim_rgn, flags); + if (ret) + goto out; } /* @@ -1856,6 +1878,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); ufshpb_sysfs_attr_show_func(rb_active_cnt); ufshpb_sysfs_attr_show_func(rb_inactive_cnt); ufshpb_sysfs_attr_show_func(map_req_cnt); +ufshpb_sysfs_attr_show_func(umap_req_cnt); static struct attribute *hpb_dev_stat_attrs[] = { &dev_attr_hit_cnt.attr, @@ -1864,6 +1887,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { &dev_attr_rb_active_cnt.attr, &dev_attr_rb_inactive_cnt.attr, &dev_attr_map_req_cnt.attr, + &dev_attr_umap_req_cnt.attr, NULL, }; @@ -1988,6 +2012,7 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb) hpb->stats.rb_active_cnt = 0; hpb->stats.rb_inactive_cnt = 0; hpb->stats.map_req_cnt = 0; + hpb->stats.umap_req_cnt = 0; } static void ufshpb_param_init(struct ufshpb_lu *hpb) diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h index 87495e59fcf1..1ea58c17a4de 100644 --- a/drivers/scsi/ufs/ufshpb.h +++ b/drivers/scsi/ufs/ufshpb.h @@ -191,6 +191,7 @@ struct ufshpb_stats { u64 rb_inactive_cnt; u64 map_req_cnt; u64 pre_req_cnt; + u64 umap_req_cnt; }; struct ufshpb_lu {
> > >> On 2021-04-06 13:20, Avri Altman wrote: > > >> >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, > > >> >> > - struct ufshpb_region *rgn) > > >> >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, > > >> >> > + struct ufshpb_region *rgn) > > >> >> > { > > >> >> > struct victim_select_info *lru_info; > > >> >> > struct ufshpb_subregion *srgn; > > >> >> > int srgn_idx; > > >> >> > > > >> >> > + lockdep_assert_held(&hpb->rgn_state_lock); > > >> >> > + > > >> >> > + if (hpb->is_hcm) { > > >> >> > + unsigned long flags; > > >> >> > + int ret; > > >> >> > + > > >> >> > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); > > >> >> > > >> >> Never seen a usage like this... Here flags is used without being > > >> >> intialized. > > >> >> The flag is needed when spin_unlock_irqrestore -> > > >> >> local_irq_restore(flags) to > > >> >> restore the DAIF register (in terms of ARM). > > >> > OK. > > >> > > >> Hi Avri, > > >> > > >> Checked on my setup, this lead to compilation error. Will you fix it > > >> in > > >> next version? > > >> > > >> warning: variable 'flags' is uninitialized when used here > > >> [-Wuninitialized] > > > Yeah - I will pass it to __ufshpb_evict_region and drop the > > > lockdep_assert call. Please ignore it. This of course won't do. Will fix it in v8. Thanks, Avri
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c index aefb6dc160ee..fcc954f51bcf 100644 --- a/drivers/scsi/ufs/ufshpb.c +++ b/drivers/scsi/ufs/ufshpb.c @@ -914,6 +914,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb, blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn); + hpb->stats.umap_req_cnt++; return 0; } @@ -1110,18 +1111,37 @@ static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb, return -EAGAIN; } +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, + struct ufshpb_region *rgn) +{ + return ufshpb_issue_umap_req(hpb, rgn); +} + static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) { return ufshpb_issue_umap_req(hpb, NULL); } -static void __ufshpb_evict_region(struct ufshpb_lu *hpb, - struct ufshpb_region *rgn) +static int __ufshpb_evict_region(struct ufshpb_lu *hpb, + struct ufshpb_region *rgn) { struct victim_select_info *lru_info; struct ufshpb_subregion *srgn; int srgn_idx; + lockdep_assert_held(&hpb->rgn_state_lock); + + if (hpb->is_hcm) { + unsigned long flags; + int ret; + + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); + ret = ufshpb_issue_umap_single_req(hpb, rgn); + spin_lock_irqsave(&hpb->rgn_state_lock, flags); + if (ret) + return ret; + } + lru_info = &hpb->lru_info; dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx); @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb, for_each_sub_region(rgn, srgn_idx, srgn) ufshpb_purge_active_subregion(hpb, srgn); + + return 0; } static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn) @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn) goto out; } - __ufshpb_evict_region(hpb, rgn); + ret = __ufshpb_evict_region(hpb, rgn); } out: spin_unlock_irqrestore(&hpb->rgn_state_lock, flags); @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn) "LRU full (%d), choose victim %d\n", atomic_read(&lru_info->active_cnt), victim_rgn->rgn_idx); - __ufshpb_evict_region(hpb, victim_rgn); + ret = __ufshpb_evict_region(hpb, victim_rgn); + if (ret) + goto out; } /* @@ -1856,6 +1880,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); ufshpb_sysfs_attr_show_func(rb_active_cnt); ufshpb_sysfs_attr_show_func(rb_inactive_cnt); ufshpb_sysfs_attr_show_func(map_req_cnt); +ufshpb_sysfs_attr_show_func(umap_req_cnt); static struct attribute *hpb_dev_stat_attrs[] = { &dev_attr_hit_cnt.attr, @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { &dev_attr_rb_active_cnt.attr, &dev_attr_rb_inactive_cnt.attr, &dev_attr_map_req_cnt.attr, + &dev_attr_umap_req_cnt.attr, NULL, }; @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb) hpb->stats.rb_active_cnt = 0; hpb->stats.rb_inactive_cnt = 0; hpb->stats.map_req_cnt = 0; + hpb->stats.umap_req_cnt = 0; } static void ufshpb_param_init(struct ufshpb_lu *hpb) diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h index 87495e59fcf1..1ea58c17a4de 100644 --- a/drivers/scsi/ufs/ufshpb.h +++ b/drivers/scsi/ufs/ufshpb.h @@ -191,6 +191,7 @@ struct ufshpb_stats { u64 rb_inactive_cnt; u64 map_req_cnt; u64 pre_req_cnt; + u64 umap_req_cnt; }; struct ufshpb_lu {
In host mode, the host is expected to send HPB-WRITE-BUFFER with buffer-id = 0x1 when it inactivates a region. Use the map-requests pool as there is no point in assigning a designated cache for umap-requests. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshpb.c | 35 +++++++++++++++++++++++++++++++---- drivers/scsi/ufs/ufshpb.h | 1 + 2 files changed, 32 insertions(+), 4 deletions(-)