Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: cang=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 17 Mar 2021 18:56:28 +0800
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 v5 06/10] scsi: ufshpb: Add hpb dev reset response
In-Reply-To: <20210302132503.224670-7-avri.altman@wdc.com>
References: <20210302132503.224670-1-avri.altman@wdc.com>
 <20210302132503.224670-7-avri.altman@wdc.com>
Message-ID: <59a62fc17ec9229a8498e696eb0474be@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

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?

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;