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;
> }
>
