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:   Mon, 15 Mar 2021 14:40:56 +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: <9d9237bc77332bd1f9ff17aaf98718a8@codeaurora.org>
References: <20210302132503.224670-1-avri.altman@wdc.com>
 <20210302132503.224670-7-avri.altman@wdc.com>
 <9d9237bc77332bd1f9ff17aaf98718a8@codeaurora.org>
Message-ID: <8b4e8dc1fd91f52a191d681b7096ef48@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2021-03-15 09:34, Can Guo wrote:
> 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;
>                         bool need_reset = false;
>> +
>> +			__shost_for_each_device(sdev, hba->host) {
>> +				struct ufshpb_lu *h = sdev->hostdata;
>> +
>> +				if (!h)
>> +					continue;
>> +
>> +				need_reset = true;
>> +			}
> 
>                         if (need_reset)
>                             schedule_work(&hpb->ufshpb_lun_reset_work);
> 
> At last, scheduling only one reset work shall be enough, otherwise 
> multiple
> reset work can be flying in parallel, so maybe above changes?

Forget about this one, I misunderstood it - reset work is for each 
ufshpb_lu...

Regards,
Can Guo.

> 
>> +		}
>> +
>>  		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)
>> +{
>> +	struct ufshpb_lu *hpb;
> 
>         struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu,
> ufshpb_lun_reset_work);
> 
>> +	struct victim_select_info *lru_info;
> 
>         struct victim_select_info *lru_info = &hpb->lru_info;
> 
> This can save some lines.
> 
> Thanks,
> Can Guo.
> 
>> +	struct ufshpb_region *rgn;
>> +	unsigned long flags;
>> +
>> +	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;