Re: [PATCH v19 1/2] scsi: ufs: Enable power management for wlun

[Adrian Hunter]

On 16/04/21 12:22 pm, Adrian Hunter wrote:
> On 16/04/21 2:36 am, Asutosh Das wrote:
>> During runtime-suspend of ufs host, the scsi devices are
>> already suspended and so are the queues associated with them.
>> But the ufs host sends SSU (START_STOP_UNIT) to wlun
>> during its runtime-suspend.
>> During the process blk_queue_enter checks if the queue is not in
>> suspended state. If so, it waits for the queue to resume, and never
>> comes out of it.
>> The commit
>> (d55d15a33: scsi: block: Do not accept any requests while suspended)
>> adds the check if the queue is in suspended state in blk_queue_enter().
>>
>> Call trace:
>>  __switch_to+0x174/0x2c4
>>  __schedule+0x478/0x764
>>  schedule+0x9c/0xe0
>>  blk_queue_enter+0x158/0x228
>>  blk_mq_alloc_request+0x40/0xa4
>>  blk_get_request+0x2c/0x70
>>  __scsi_execute+0x60/0x1c4
>>  ufshcd_set_dev_pwr_mode+0x124/0x1e4
>>  ufshcd_suspend+0x208/0x83c
>>  ufshcd_runtime_suspend+0x40/0x154
>>  ufshcd_pltfrm_runtime_suspend+0x14/0x20
>>  pm_generic_runtime_suspend+0x28/0x3c
>>  __rpm_callback+0x80/0x2a4
>>  rpm_suspend+0x308/0x614
>>  rpm_idle+0x158/0x228
>>  pm_runtime_work+0x84/0xac
>>  process_one_work+0x1f0/0x470
>>  worker_thread+0x26c/0x4c8
>>  kthread+0x13c/0x320
>>  ret_from_fork+0x10/0x18
>>
>> Fix this by registering ufs device wlun as a scsi driver and
>> registering it for block runtime-pm. Also make this as a
>> supplier for all other luns. That way, this device wlun
>> suspends after all the consumers and resumes after
>> hba resumes.
>> This also registers a new scsi driver for rpmb wlun.
>> This new driver is mostly used to clear rpmb uac.
>> With this design, the driver would always be runtime resumed
>> before system suspend.
> 
> I thought some more about that and I think we can still support
> allowing runtime suspend to work with system suspend, without
> too much difficulty. See ufshcd_suspend_prepare() below.
> 
>>
>> Fixed smatch warnings:
>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>>
>> Co-developed-by: Can Guo <cang@xxxxxxxxxxxxxx>
>> Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
>> Signed-off-by: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
>> ---
> 
> <SNIP>
> 
>> -static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>> +static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>  {
>>  	int ret;
>> -	enum uic_link_state old_link_state;
>> +	enum uic_link_state old_link_state = hba->uic_link_state;
>>  
>> -	hba->pm_op_in_progress = 1;
>> -	old_link_state = hba->uic_link_state;
>> -
>> -	ufshcd_hba_vreg_set_hpm(hba);
>> -	ret = ufshcd_vreg_set_hpm(hba);
>> -	if (ret)
>> -		goto out;
>> -
>> -	/* Make sure clocks are enabled before accessing controller */
>> -	ret = ufshcd_setup_clocks(hba, true);
>> -	if (ret)
>> -		goto disable_vreg;
>> -
>> -	/* enable the host irq as host controller would be active soon */
>> -	ufshcd_enable_irq(hba);
>> +	hba->pm_op_in_progress = true;
>>  
>>  	/*
>>  	 * Call vendor specific resume callback. As these callbacks may access
>> @@ -8868,7 +8858,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>  	 */
>>  	ret = ufshcd_vops_resume(hba, pm_op);
>>  	if (ret)
>> -		goto disable_irq_and_vops_clks;
>> +		goto out;
>>  
>>  	/* For DeepSleep, the only supported option is to have the link off */
>>  	WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) && !ufshcd_is_link_off(hba));
>> @@ -8916,42 +8906,219 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>  	if (hba->ee_usr_mask)
>>  		ufshcd_write_ee_control(hba);
>>  
>> -	hba->clk_gating.is_suspended = false;
>> -
>>  	if (ufshcd_is_clkscaling_supported(hba))
>> -		ufshcd_clk_scaling_suspend(hba, false);
>> -
>> -	/* Enable Auto-Hibernate if configured */
>> -	ufshcd_auto_hibern8_enable(hba);
>> +		ufshcd_resume_clkscaling(hba);
> 
> This still doesn't look right. ufshcd_resume_clkscaling()
> doesn't update hba->clk_scaling.is_allowed whereas
> ufshcd_clk_scaling_suspend() does.
> 
>>  
>>  	if (hba->dev_info.b_rpm_dev_flush_capable) {
>>  		hba->dev_info.b_rpm_dev_flush_capable = false;
>>  		cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
>>  	}
>>  
>> -	ufshcd_clear_ua_wluns(hba);
>> -
>> -	/* Schedule clock gating in case of no access to UFS device yet */
>> -	ufshcd_release(hba);
>> -
>> +	/* Enable Auto-Hibernate if configured */
>> +	ufshcd_auto_hibern8_enable(hba);
>>  	goto out;
>>  
>>  set_old_link_state:
>>  	ufshcd_link_state_transition(hba, old_link_state, 0);
>>  vendor_suspend:
>>  	ufshcd_vops_suspend(hba, pm_op);
>> -disable_irq_and_vops_clks:
>> +out:
>> +	if (ret)
>> +		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
>> +	hba->clk_gating.is_suspended = false;
>> +	ufshcd_release(hba);
>> +	hba->pm_op_in_progress = false;
>> +	return ret;
>> +}
> 
> <SNIP>
> 
>> +void ufshcd_resume_complete(struct device *dev)
>> +{
>> +	struct ufs_hba *hba = dev_get_drvdata(dev);
>> +
>> +	ufshcd_rpm_put(hba);
>> +}
>> +EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
>> +
>> +int ufshcd_suspend_prepare(struct device *dev)
>> +{
>> +	struct ufs_hba *hba = dev_get_drvdata(dev);
>> +
>> +	/*
>> +	 * SCSI assumes that runtime-pm and system-pm for scsi drivers
>> +	 * are same. And it doesn't wake up the device for system-suspend
>> +	 * if it's runtime suspended. But ufs doesn't follow that.
>> +	 * The rpm-lvl and spm-lvl can be different in ufs.
>> +	 * Force it to honor system-suspend.
>> +	 * Refer ufshcd_resume_complete()
>> +	 */
>> +	ufshcd_rpm_get_sync(hba);
>> +
>> +	return 0;
>> +}
> 
> I think we can support allowing runtime suspend to work with
> system suspend.  ufshcd_resume_complete() remains the same,
> and ufshcd_suspend_prepare() is like this:
> 
> 
> /*
>  * SCSI assumes that runtime-pm and system-pm for scsi drivers are same, and it
>  * doesn't wake up the device for system-suspend if it's runtime suspended.
>  * However UFS doesn't follow that. The rpm-lvl and spm-lvl can be different in
>  * UFS, so special care is needed.
>  * Refer also ufshcd_resume_complete()
>  */
> int ufshcd_suspend_prepare(struct device *dev)
> {
> 	struct ufs_hba *hba = dev_get_drvdata(dev);
> 	struct device *ufs_dev = &hba->sdev_ufs_device->sdev_gendev;
> 	enum ufs_dev_pwr_mode spm_pwr_mode;
> 	enum uic_link_state spm_link_state;
> 	unsigned long flags;
> 	bool rpm_state_ok;
> 
> 	/*
> 	 * First prevent runtime suspend. Note this does not prevent runtime
> 	 * resume e.g. pm_runtime_get_sync() will still do the right thing.
> 	 */
> 	pm_runtime_get_noresume(ufs_dev);
> 
> 	/* Now check if the rpm state is ok to use for spm */
> 	spin_lock_irqsave(&ufs_dev->power.lock, flags);
> 
> 	spm_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl);
> 	spm_link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl);
> 
> 	rpm_state_ok = pm_runtime_suspended(ufs_dev) &&
> 		       hba->curr_dev_pwr_mode == spm_pwr_mode &&
> 		       hba->uic_link_state == spm_link_state &&
> 		       !hba->dev_info.b_rpm_dev_flush_capable;
> 
> 	spin_unlock_irqrestore(&ufs_dev->power.lock, flags);
> 
> 	/* If is isn't, do a runtime resume */
> 	if (!rpm_state_ok)
> 		pm_runtime_resume(ufs_dev);

But we should return an error if runtime resume
fails.

 	if (!rpm_state_ok) {
 		int ret = pm_runtime_resume(ufs_dev);

		if (ret < 0 && ret != EACCES) {
			pm_runtime_put(ufs_dev);
			return ret;
		}
	}

> 
> 	return 0;
> }
>