[PATCH] Revert "scsi: ufs: core: Initialize devfreq synchronously"

[PATCH] Revert "scsi: ufs: core: Initialize devfreq synchronously"

[Adrien Thierry]

This reverts commit 7dafc3e007918384c8693ff8d70381b5c1e9c247.

This patch introduced a regression [1] where hba->pwr_info is used
before being initialized, which could create issues in
ufshcd_scale_gear(). Revert it until a better solution is found.

[1] https://lore.kernel.org/all/CAGaU9a_PMZhqv+YJ0r3w-hJMsR922oxW6Kg59vw+oen-NZ6Otw@mail.gmail.com

Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 drivers/ufs/core/ufshcd.c | 47 +++++++++++++--------------------------
 include/ufs/ufshcd.h      |  1 -
 2 files changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 37e178a9ac47..70b112038792 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1409,13 +1409,6 @@ static int ufshcd_devfreq_target(struct device *dev,
 	struct ufs_clk_info *clki;
 	unsigned long irq_flags;
 
-	/*
-	 * Skip devfreq if UFS initialization is not finished.
-	 * Otherwise ufs could be in a inconsistent state.
-	 */
-	if (!smp_load_acquire(&hba->logical_unit_scan_finished))
-		return 0;
-
 	if (!ufshcd_is_clkscaling_supported(hba))
 		return -EINVAL;
 
@@ -8399,6 +8392,22 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 	if (ret)
 		goto out;
 
+	/* Initialize devfreq after UFS device is detected */
+	if (ufshcd_is_clkscaling_supported(hba)) {
+		memcpy(&hba->clk_scaling.saved_pwr_info.info,
+			&hba->pwr_info,
+			sizeof(struct ufs_pa_layer_attr));
+		hba->clk_scaling.saved_pwr_info.is_valid = true;
+		hba->clk_scaling.is_allowed = true;
+
+		ret = ufshcd_devfreq_init(hba);
+		if (ret)
+			goto out;
+
+		hba->clk_scaling.is_enabled = true;
+		ufshcd_init_clk_scaling_sysfs(hba);
+	}
+
 	ufs_bsg_probe(hba);
 	ufshpb_init(hba);
 	scsi_scan_host(hba->host);
@@ -8670,12 +8679,6 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	if (ret) {
 		pm_runtime_put_sync(hba->dev);
 		ufshcd_hba_exit(hba);
-	} else {
-		/*
-		 * Make sure that when reader code sees UFS initialization has finished,
-		 * all initialization steps have really been executed.
-		 */
-		smp_store_release(&hba->logical_unit_scan_finished, true);
 	}
 }
 
@@ -10316,30 +10319,12 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	 */
 	ufshcd_set_ufs_dev_active(hba);
 
-	/* Initialize devfreq */
-	if (ufshcd_is_clkscaling_supported(hba)) {
-		memcpy(&hba->clk_scaling.saved_pwr_info.info,
-			&hba->pwr_info,
-			sizeof(struct ufs_pa_layer_attr));
-		hba->clk_scaling.saved_pwr_info.is_valid = true;
-		hba->clk_scaling.is_allowed = true;
-
-		err = ufshcd_devfreq_init(hba);
-		if (err)
-			goto rpm_put_sync;
-
-		hba->clk_scaling.is_enabled = true;
-		ufshcd_init_clk_scaling_sysfs(hba);
-	}
-
 	async_schedule(ufshcd_async_scan, hba);
 	ufs_sysfs_add_nodes(hba->dev);
 
 	device_enable_async_suspend(dev);
 	return 0;
 
-rpm_put_sync:
-	pm_runtime_put_sync(dev);
 free_tmf_queue:
 	blk_mq_destroy_queue(hba->tmf_queue);
 	blk_put_queue(hba->tmf_queue);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 25aab8ec4f86..431c3afb2ce0 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -979,7 +979,6 @@ struct ufs_hba {
 	struct completion *uic_async_done;
 
 	enum ufshcd_state ufshcd_state;
-	bool logical_unit_scan_finished;
 	u32 eh_flags;
 	u32 intr_mask;
 	u16 ee_ctrl_mask;
-- 
2.39.2

Re: [PATCH] Revert "scsi: ufs: core: Initialize devfreq synchronously"

[Stanley Chu]

Hi Adrien,

On Thu, Mar 30, 2023 at 4:54 AM Adrien Thierry <athierry@redhat.com> wrote:
>
> This reverts commit 7dafc3e007918384c8693ff8d70381b5c1e9c247.
>
> This patch introduced a regression [1] where hba->pwr_info is used
> before being initialized, which could create issues in
> ufshcd_scale_gear(). Revert it until a better solution is found.
>
> [1] https://lore.kernel.org/all/CAGaU9a_PMZhqv+YJ0r3w-hJMsR922oxW6Kg59vw+oen-NZ6Otw@mail.gmail.com
>
> Signed-off-by: Adrien Thierry <athierry@redhat.com>

Thank you for posting the revert patch in such a short time.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH] Revert "scsi: ufs: core: Initialize devfreq synchronously"

[Martin K. Petersen]

On Wed, 29 Mar 2023 16:54:25 -0400, Adrien Thierry wrote:

> This reverts commit 7dafc3e007918384c8693ff8d70381b5c1e9c247.
> 
> This patch introduced a regression [1] where hba->pwr_info is used
> before being initialized, which could create issues in
> ufshcd_scale_gear(). Revert it until a better solution is found.
> 
> [1] https://lore.kernel.org/all/CAGaU9a_PMZhqv+YJ0r3w-hJMsR922oxW6Kg59vw+oen-NZ6Otw@mail.gmail.com
> 
> [...]

Applied to 6.3/scsi-fixes, thanks!

[1/1] Revert "scsi: ufs: core: Initialize devfreq synchronously"
      https://git.kernel.org/mkp/scsi/c/86eb94bf8006

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "scsi: ufs: core: Initialize devfreq synchronously"

[Adrien Thierry]

Hi,

On Wed, Mar 29, 2023 at 04:54:25PM -0400, Adrien Thierry wrote:
> This reverts commit 7dafc3e007918384c8693ff8d70381b5c1e9c247.
> 
> This patch introduced a regression [1] where hba->pwr_info is used
> before being initialized, which could create issues in
> ufshcd_scale_gear(). Revert it until a better solution is found.
> 
> [1] https://lore.kernel.org/all/CAGaU9a_PMZhqv+YJ0r3w-hJMsR922oxW6Kg59vw+oen-NZ6Otw@mail.gmail.com
> 
> Signed-off-by: Adrien Thierry <athierry@redhat.com>
> ---
>  drivers/ufs/core/ufshcd.c | 47 +++++++++++++--------------------------
>  include/ufs/ufshcd.h      |  1 -
>  2 files changed, 16 insertions(+), 32 deletions(-)
>

I've been working on a fixed version, and realized the original issue [1]
has been fixed by [2]. Thanks to the softdep, whenever the ufs core and
the simple ondemand governor are built as modules, the governor module
will be loaded before the ufs core module. Thus, the ufs core doesn't end
up calling request_module because the governor is already loaded.

So, it looks like there's now no need to submit a fixed version.

[1] https://lore.kernel.org/all/20230217194423.42553-1-athierry@redhat.com/
[2] https://lore.kernel.org/all/20230220140740.14379-1-athierry@redhat.com/

Best,

Adrien