Re: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

From: Can Guo <cang@codeaurora.org>
To: Avri Altman <avri.altman@wdc.com>
Cc: "James E . J . Bottomley" <jejb@linux.vnet.ibm.com>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
	gregkh@linuxfoundation.org, Bart Van Assche <bvanassche@acm.org>,
	yongmyung lee <ymhungry.lee@samsung.com>,
	Daejun Park <daejun7.park@samsung.com>,
	alim.akhtar@samsung.com, asutoshd@codeaurora.org,
	Zang Leigang <zangleigang@hisilicon.com>,
	Avi Shchislowski <avi.shchislowski@wdc.com>,
	Bean Huo <beanhuo@micron.com>,
	stanley.chu@mediatek.com
Subject: Re: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode
Date: Tue, 06 Apr 2021 12:53:29 +0800	[thread overview]
Message-ID: <e29e33769f23036f936a6b60c7430387@codeaurora.org> (raw)
In-Reply-To: <20210331073952.102162-7-avri.altman@wdc.com>

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 {