On 16/04/21 12:22 pm, Adrian Hunter wrote:

On 16/04/21 2:36 am, Asutosh Das wrote:

quoted

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.

quoted

Fixed smatch warnings:
Reported-by: kernel test robot <redacted>
Reported-by: Dan Carpenter <redacted>

Co-developed-by: Can Guo <redacted>
Signed-off-by: Can Guo <redacted>
Signed-off-by: Asutosh Das <redacted>
---

<SNIP>

quoted

-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.

quoted

 
 	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>

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel